Adding support for the package attribute on the types element. #904

ratcashdev · 2022-07-05T20:43:15Z

Implementation of #507 for Java.

The main idea is that we store the package attribute read from the XML to the IR, into token[0]. Then it's up to the generator to make use of that extra piece of information (controllable through sbe.type.package.override argument, defaulting to false ).

The serialization of the IR is not touched, therefore the package information is not present in the serialized IR.

Some classes in this PR, like the MultiPackageOutputManager and StringWriterOutputManager actually belong to agrona, but have been implemented here to have a testable code.
Before merging, those files/classes should be moved and substituted with a new release of agrona.

ratcashdev · 2022-07-27T10:15:26Z

@mjpt777 @tmontgomery Hi, subject to Your time permitting, I'd appreciate a 1st, rough review of this. Thanks.

mjpt777 · 2022-07-27T12:22:19Z

Sorry but we have been busy. We need to give priority to our commercial support customers.

ratcashdev · 2022-08-08T12:08:42Z

Sure, take your time. Just wanted to make sure this blipped on your radar.

mjpt777 · 2022-08-09T14:00:49Z

sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/TargetCodeGeneratorLoader.java

@@ -52,7 +52,8 @@ public CodeGenerator newInstance(final Ir ir, final String outputDir)
                "true".equals(System.getProperty(JAVA_GROUP_ORDER_ANNOTATION)),
                "true".equals(System.getProperty(JAVA_GENERATE_INTERFACES)),
                "true".equals(System.getProperty(DECODE_UNKNOWN_ENUM_VALUES)),
-                new PackageOutputManager(outputDir, ir.applicableNamespace()));
+                "true".equals(System.getProperty(TYPE_PACKAGE_OVERRIDE)),


Should this default to true?

unless TYPE_PACKAGE_OVERRIDE is explicitly set to true, the generator will not respect package overrides, therefore behave like before this change - unless I am missing something here?

sbe-tool/src/main/java/uk/co/real_logic/sbe/xml/Type.java

sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/java/JavaOutputManager.java

mjpt777 · 2022-08-09T14:08:52Z

sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/java/JavaOutputManager.java

+{
+
+    private final String baseDirName;
+    private final PackageOutputManager global;


Naming should be more explicit.

Naming's difficult. But tried to do my best.

mjpt777 · 2022-08-09T14:09:37Z

sbe-tool/src/main/java/uk/co/real_logic/sbe/ir/Token.java

@@ -105,6 +107,7 @@
    public Token(
        final Signal signal,
        final String name,
+        final String packageName,


Should come at end of string args as new.

mjpt777 · 2022-08-09T14:11:23Z

sbe-tool/src/main/java/uk/co/real_logic/sbe/ir/Token.java

@@ -361,6 +375,7 @@ public String toString()
    {
        private Signal signal;
        private String name;
+        private String packageName;


What is the default for packageName?

Set the default to null. Also added explanation to the javadoc.

sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/java/JavaOutputManager.java

ratcashdev · 2022-09-05T09:33:19Z

Created real-logic/agrona#266 for the required changes in agrona.

…putManager). Should be replaced with a new version of Agrona.

vyazelenko · 2022-09-14T16:49:38Z

@ratcashdev Agrona 1.17.0 was released and it contains real-logic/agrona#266.

ratcashdev · 2022-09-14T18:22:40Z

updated to latest master.

mjpt777 · 2022-09-23T12:39:28Z

sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/java/JavaOutputManager.java

+        actingPackageOutputManager = basePackageOutputManager;
+    }
+
+    @Override


@OverRide not used in code style.

mjpt777 · 2022-09-28T06:46:18Z

sbe-tool/src/main/java/uk/co/real_logic/sbe/xml/XmlSchemaParser.java

+     */
+    public static String getTypesPackageAttribute(final Node elementNode)
+    {
+        return getAttributeValue(elementNode.getParentNode(), "package", null);


This is not likely to be sufficient for nested composites or ref types as the parent will be different.

True. I need to write a test case for this. So we speak about two cases as below?

... <messageSchema package="composite.elements" id="3" semanticVersion="5.2" description="Unit Test" byteOrder="littleEndian"> <types package="specific.package.name"> <composite name="messageHeader" description="Message identifiers and length of message root"> <type name="blockLength" primitiveType="uint16"/> <type name="templateId" primitiveType="uint16"/> <type name="schemaId" primitiveType="uint16"/> <type name="version" primitiveType="uint16"/> </composite> ...  <enum name="booleanEnum" encodingType="uint8" semanticType="Boolean"> <validValue name="F">0</validValue> <validValue name="T">1</validValue> </enum> <composite name="futuresPrice"> <type name="mantissa" primitiveType="int64" /> <type name="exponent" primitiveType="int8" /> <ref name="isSettlement" type="booleanEnum" offset="10" /> </composite>  <composite name="outer"> <enum name="enumOne" description="enum as uint8" encodingType="uint8" semanticType="int"> <validValue name="Value1">1</validValue> <validValue name="Value10">10</validValue> </enum> <type name="zeroth" primitiveType="uint8"/> <set name="setOne" description="set as uint32" encodingType="uint32" semanticType="int"> <choice name="Bit0">0</choice> <choice name="Bit16">16</choice> <choice name="Bit26">26</choice> </set> <composite name="inner"> <type name="first" primitiveType="int64"/> <type name="second" primitiveType="int64"/> </composite> </composite> </types> ...

Yes these cases and consider the likes of: https://github.com/real-logic/simple-binary-encoding/blob/master/sbe-samples/src/main/resources/example-schema.xml#L32-L34

BTW I've pushed a change to better find the ancestor <types> element.

ratcashdev · 2022-09-28T08:48:41Z

Ok, will enhance the tests as a separate PR. Thanks for the fix.

ratcashdev · 2022-09-28T11:12:30Z

just realized, the case below also needs to be taken care of or at least tested (REF pointing to a type in a different package):

 <types package="specific.package.name.A">
       <enum name="booleanEnum" encodingType="uint8" semanticType="Boolean">
            <validValue name="F">0</validValue>
            <validValue name="T">1</validValue>
        </enum>
</types>

 <types package="specific.package.name.B">
       <composite name="futuresPrice">
            <type name="mantissa" primitiveType="int64" />
            <type name="exponent" primitiveType="int8" />
            <ref name="isSettlement" type="booleanEnum" offset="10" />
        </composite>
</types>

mjpt777 · 2022-09-29T12:35:35Z

Yes the <ref> case needs to be considered.

mjpt777 · 2022-10-03T10:07:25Z

When do you think you can address this?

ratcashdev · 2022-10-03T10:21:23Z

Apologies, got sick for the past week, still on meds. Fingers cross - this week.

ratcashdev · 2022-10-03T13:27:24Z

please have a look on #915

ratcashdev added 2 commits July 5, 2022 22:06

Adding support for the package attribute on the types element.

20efc51

checkstyle.

2ac9ee3

ratcashdev marked this pull request as ready for review July 18, 2022 23:01

mjpt777 reviewed Aug 9, 2022

View reviewed changes

sbe-tool/src/main/java/uk/co/real_logic/sbe/xml/Type.java Outdated Show resolved Hide resolved

mjpt777 reviewed Aug 9, 2022

View reviewed changes

sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/java/JavaOutputManager.java Outdated Show resolved Hide resolved

mjpt777 reviewed Aug 9, 2022

View reviewed changes

sbe-tool/src/main/java/uk/co/real_logic/sbe/generation/java/JavaOutputManager.java Outdated Show resolved Hide resolved

ratcashdev added 5 commits August 10, 2022 10:37

Revert accidental delete.

7280689

Change parameter order for Token. Add Javadoc.

81eb134

Code style fixes.

e9b4134

Merge remote-tracking branch 'origin/master'

065e6b6

Make QualifiedYieldTest use the new constructor.

f1eff14

ratcashdev requested a review from mjpt777 August 30, 2022 13:01

ratcashdev mentioned this pull request Sep 2, 2022

DynamicPackageOutputManager and its implementation real-logic/agrona#266

Merged

Revert changes to test files (using custom version of StringWriterOut…

b021a02

…putManager). Should be replaced with a new version of Agrona.

ratcashdev added 3 commits September 14, 2022 19:23

Merge remote-tracking branch 'origin/master'

8ceb73d

Use agrone 1.17.1

b29daa1

Merge remote-tracking branch 'origin/master'

b38f1cd

mjpt777 reviewed Sep 23, 2022

View reviewed changes

ratcashdev added 2 commits September 23, 2022 16:43

Remove @OverRide

6ddce5a

make checkstyle happy.

f876441

Correct inherit directive.

6c0a263

mjpt777 merged commit 742cd8a into real-logic:master Sep 27, 2022

mjpt777 added a commit that referenced this pull request Sep 27, 2022

[Java] Clean up after merge of PR #904.

a52aef5

mjpt777 reviewed Sep 28, 2022

View reviewed changes

mjpt777 added a commit that referenced this pull request Sep 28, 2022

[Java] Fix lookup of types ancestor for PR #904.

ff5158e

ratcashdev mentioned this pull request Oct 3, 2022

Fix package name support for embedded composites #915

Merged

vyazelenko mentioned this pull request Nov 13, 2024

[C#,C++,Java] Generate DTOs for non-perf-sensitive usecases. #957

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for the package attribute on the types element. #904

Adding support for the package attribute on the types element. #904

ratcashdev commented Jul 5, 2022 •

edited

Loading

ratcashdev commented Jul 27, 2022

mjpt777 commented Jul 27, 2022

ratcashdev commented Aug 8, 2022

mjpt777 Aug 9, 2022

ratcashdev Aug 10, 2022 •

edited

Loading

mjpt777 Aug 9, 2022

ratcashdev Aug 10, 2022

mjpt777 Aug 9, 2022

ratcashdev Aug 10, 2022

mjpt777 Aug 9, 2022

ratcashdev Aug 10, 2022

ratcashdev commented Sep 5, 2022

vyazelenko commented Sep 14, 2022

ratcashdev commented Sep 14, 2022

mjpt777 Sep 23, 2022

mjpt777 Sep 28, 2022

ratcashdev Sep 28, 2022 •

edited

Loading

mjpt777 Sep 28, 2022

ratcashdev commented Sep 28, 2022

ratcashdev commented Sep 28, 2022 •

edited

Loading

mjpt777 commented Sep 29, 2022

mjpt777 commented Oct 3, 2022

ratcashdev commented Oct 3, 2022

ratcashdev commented Oct 3, 2022

Adding support for the package attribute on the types element. #904

Adding support for the package attribute on the types element. #904

Conversation

ratcashdev commented Jul 5, 2022 • edited Loading

ratcashdev commented Jul 27, 2022

mjpt777 commented Jul 27, 2022

ratcashdev commented Aug 8, 2022

Choose a reason for hiding this comment

ratcashdev Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ratcashdev commented Sep 5, 2022

vyazelenko commented Sep 14, 2022

ratcashdev commented Sep 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ratcashdev Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ratcashdev commented Sep 28, 2022

ratcashdev commented Sep 28, 2022 • edited Loading

mjpt777 commented Sep 29, 2022

mjpt777 commented Oct 3, 2022

ratcashdev commented Oct 3, 2022

ratcashdev commented Oct 3, 2022

ratcashdev commented Jul 5, 2022 •

edited

Loading

ratcashdev Aug 10, 2022 •

edited

Loading

ratcashdev Sep 28, 2022 •

edited

Loading

ratcashdev commented Sep 28, 2022 •

edited

Loading