Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Java] Permit decoders to be built that do not validate empty tags #534

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[Java] Permit decoders to be built that do not validate empty tags #534

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8102fb4
[Java] Permit decoders to be built that do not validate empty tags
writeoncereadmany Dec 5, 2024
4e7e8df
[Java] Do not parse tags with empty values if we have allowed empty t…
writeoncereadmany Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions ...codecs/src/main/java/uk/co/real_logic/artio/dictionary/generation/CodecConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public final class CodecConfiguration
* without checking whether the field is set.
*/
public static final String WRAP_EMPTY_BUFFER = "fix.codecs.wrap_empty_buffer";
public static final String ALLOW_EMPTY_TAGS = "fix.codecs.allow_empty_tags";
public static final String PARENT_PACKAGE_PROPERTY = "fix.codecs.parent_package";
public static final String FLYWEIGHTS_ENABLED_PROPERTY = "fix.codecs.flyweight";
public static final String REJECT_UNKNOWN_ENUM_VALUE_PROPERTY = "reject.unknown.enum.value";
Expand All @@ -63,6 +64,7 @@ public final class CodecConfiguration
private String parentPackage = System.getProperty(PARENT_PACKAGE_PROPERTY, DEFAULT_PARENT_PACKAGE);
private boolean flyweightsEnabled = Boolean.getBoolean(FLYWEIGHTS_ENABLED_PROPERTY);
private boolean wrapEmptyBuffer = Boolean.getBoolean(WRAP_EMPTY_BUFFER);
private boolean allowEmptyTags = Boolean.getBoolean(ALLOW_EMPTY_TAGS);
private boolean fixTagsInJavadoc = Boolean.parseBoolean(System.getProperty(
FIX_TAGS_IN_JAVADOC, DEFAULT_FIX_TAGS_IN_JAVADOC));
private SharedCodecConfiguration sharedCodecConfiguration;
Expand Down Expand Up @@ -129,6 +131,21 @@ public CodecConfiguration wrapEmptyBuffer(final boolean wrapEmptyBuffer)
return this;
}

/**
* Suppresses checks for empty tags, eg |1234=|
* instead of rejecting when tag is empty, treat the tag as absent
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, because you're still parsing those fields, the hasFoo() methods will return true. I think that's dangerous, because you won't be able to tell a difference between, for example, |43=N| and |43=|.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, damn, good point. I'll revise to make sure we don't parse the tags either and properly treat them as absent.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now updated the change to not parse the tags, and added tests to assert those fields are absent (rather than present but empty/default).

I've included both String and non-String field tests too (to show it gracefully handles both parsed and non-parsed fields).

*
* Defaults to the value of {@link #ALLOW_EMPTY_TAGS} system property.
*
* @param allowEmptyTags true to suppress check of empty tags, false to keep checking (default)
* @return this
*/
public CodecConfiguration allowEmptyTags(final boolean allowEmptyTags)
{
this.allowEmptyTags = allowEmptyTags;
return this;
}

/**
* Allow duplicate fields. Executable documentation can be found in the test "DuplicateFieldsTest".
*
Expand Down Expand Up @@ -248,6 +265,11 @@ boolean wrapEmptyBuffer()
return wrapEmptyBuffer;
}

public boolean allowEmptyTags()
{
return allowEmptyTags;
}

String codecRejectUnknownEnumValueEnabled()
{
return codecRejectUnknownEnumValueEnabled;
Expand Down
2 changes: 2 additions & 0 deletions artio-codecs/src/main/java/uk/co/real_logic/artio/dictionary/generation/CodecGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ private static void generateDictionary(
RejectUnknownEnumValue.class,
false,
configuration.wrapEmptyBuffer(),
configuration.allowEmptyTags(),
codecRejectUnknownEnumValueEnabled,
configuration.fixTagsInJavadoc()).generate();

Expand All @@ -202,6 +203,7 @@ private static void generateDictionary(
RejectUnknownEnumValue.class,
true,
configuration.wrapEmptyBuffer(),
configuration.allowEmptyTags(),
codecRejectUnknownEnumValueEnabled,
configuration.fixTagsInJavadoc()).generate();
}
Expand Down
29 changes: 24 additions & 5 deletions ...o-codecs/src/main/java/uk/co/real_logic/artio/dictionary/generation/DecoderGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,11 @@ static String decoderClassName(final String name)
* Wrap empty buffer instead of throwing an exception if an optional string is unset.
*/
private final boolean wrapEmptyBuffer;
/**
* Do not reject messages when there's an empty tag: instead, treat the field as absent
*/
private final boolean allowEmptyTags;


DecoderGenerator(
final Dictionary dictionary,
Expand All @@ -132,6 +137,7 @@ static String decoderClassName(final String name)
final Class<?> rejectUnknownEnumValueClass,
final boolean flyweightsEnabled,
final boolean wrapEmptyBuffer,
final boolean allowEmptyTags,
final String codecRejectUnknownEnumValueEnabled,
final boolean fixTagsInJavadoc)
{
Expand All @@ -140,6 +146,7 @@ static String decoderClassName(final String name)
this.initialBufferSize = initialBufferSize;
this.encoderPackage = encoderPackage;
this.wrapEmptyBuffer = wrapEmptyBuffer;
this.allowEmptyTags = allowEmptyTags;
}

public void generate()
Expand Down Expand Up @@ -1770,11 +1777,7 @@ private String generateDecodePrefix(
" invalidTagId = tag;\n" +
" rejectReason = " + INVALID_TAG_NUMBER + ";\n" +
" }\n" +
" else if (valueLength == 0)\n" +
" {\n" +
" invalidTagId = tag;\n" +
" rejectReason = " + TAG_SPECIFIED_WITHOUT_A_VALUE + ";\n" +
" }\n" +
emptyTagValidation() +
headerValidation(isHeader) +
(isGroup ? "" :
" if (!alreadyVisitedFields.add(tag))\n" +
Expand All @@ -1789,6 +1792,22 @@ private String generateDecodePrefix(
" {\n";
}

private String emptyTagValidation()
{
if (allowEmptyTags)
{
return "";
}
else
{
return " else if (valueLength == 0)\n" +
" {\n" +
" invalidTagId = tag;\n" +
" rejectReason = " + TAG_SPECIFIED_WITHOUT_A_VALUE + ";\n" +
" }\n";
}
}

private String decodeTrailerOrReturn(final boolean hasCommonCompounds, final int indent)
{
return (hasCommonCompounds ?
Expand Down
4 changes: 4 additions & 0 deletions artio-codecs/src/test/java/uk/co/real_logic/artio/dictionary/ExampleDictionary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,10 @@ public final class ExampleDictionary
"8=FIX.4.4\0019=0027\00135=0\001115=abc\001116=2\001116=1\001117=1.1\001127=19700101-00:00:00.001" +
"\00110=161\001";

public static final String EMPTY_FIX_TAG_FOR_STRING_FIELD_MESSAGE =
"8=FIX.4.4\0019=81\00135=0\001115=abc\001112=\001116=2\001117=1.1" +
"\001118=Y\001200=3\001119=123\001127=19700101-00:00:00.001\00110=199\001";

public static final String DERIVED_FIELDS_MESSAGE =
"8=FIX.4.4\0019=53\00135=0\001115=abc\001116=2\001117=1.1\001127=19700101-00:00:00.001" +
"\00110=043\001";
Expand Down
32 changes: 26 additions & 6 deletions .../test/java/uk/co/real_logic/artio/dictionary/generation/AbstractDecoderGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public abstract class AbstractDecoderGeneratorTest
private static Class<?> heartbeatWithoutValidation;
private static Class<?> heartbeatWithoutEnumValueValidation;
private static Class<?> heartbeatWithRejectingUnknownFields;
private static Class<?> heartbeatAllowingEmptyTags;
private static Class<?> heartbeat;
private static Class<?> component;
private static Class<?> otherMessage;
Expand All @@ -85,13 +86,15 @@ public abstract class AbstractDecoderGeneratorTest
static void generate(final boolean flyweightStringsEnabled) throws Exception
{
sourcesWithValidation = generateSources(
true, false, true, flyweightStringsEnabled, false);
true, false, true, flyweightStringsEnabled, false, false);
final Map<String, CharSequence> sourcesWithNoEnumValueValidation = generateSources(
true, false, false, flyweightStringsEnabled, false);
true, false, false, flyweightStringsEnabled, false, false);
final Map<String, CharSequence> sourcesWithoutValidation = generateSources(
false, false, true, flyweightStringsEnabled, true);
false, false, true, flyweightStringsEnabled, true, false);
final Map<String, CharSequence> sourcesRejectingUnknownFields = generateSources(
true, true, true, flyweightStringsEnabled, false);
true, true, true, flyweightStringsEnabled, false, false);
final Map<String, CharSequence> sourcesAllowingEmptyTags = generateSources(
true, false, true, flyweightStringsEnabled, false, true);
heartbeat = compileInMemory(HEARTBEAT_DECODER, sourcesWithValidation);
if (heartbeat == null || CODEC_LOGGING)
{
Expand All @@ -109,6 +112,7 @@ static void generate(final boolean flyweightStringsEnabled) throws Exception
heartbeatWithoutValidation = compileInMemory(HEARTBEAT_DECODER, sourcesWithoutValidation);
heartbeatWithoutEnumValueValidation = compileInMemory(HEARTBEAT_DECODER, sourcesWithNoEnumValueValidation);
heartbeatWithRejectingUnknownFields = compileInMemory(HEARTBEAT_DECODER, sourcesRejectingUnknownFields);
heartbeatAllowingEmptyTags = compileInMemory(HEARTBEAT_DECODER, sourcesAllowingEmptyTags);
allReqFieldTypesMessage = compileInMemory(ALL_REQ_FIELD_TYPES_MESSAGE_DECODER, sourcesWithoutValidation);
if (heartbeatWithoutValidation == null || CODEC_LOGGING)
{
Expand All @@ -118,7 +122,8 @@ static void generate(final boolean flyweightStringsEnabled) throws Exception

private static Map<String, CharSequence> generateSources(
final boolean validation, final boolean rejectingUnknownFields, final boolean rejectingUnknownEnumValue,
final boolean flyweightStringsEnabled, final boolean wrapEmptyBuffer)
final boolean flyweightStringsEnabled, final boolean wrapEmptyBuffer, final boolean allowEmptyTags
)
{
final Class<?> validationClass = validation ? ValidationOn.class : ValidationOff.class;
final Class<?> rejectUnknownField = rejectingUnknownFields ?
Expand All @@ -132,7 +137,7 @@ private static Map<String, CharSequence> generateSources(
final DecoderGenerator decoderGenerator = new DecoderGenerator(
MESSAGE_EXAMPLE, 1, TEST_PACKAGE, TEST_PARENT_PACKAGE, TEST_PACKAGE,
outputManager, validationClass, rejectUnknownField,
rejectUnknownEnumValue, flyweightStringsEnabled, wrapEmptyBuffer,
rejectUnknownEnumValue, flyweightStringsEnabled, wrapEmptyBuffer, allowEmptyTags,
String.valueOf(rejectingUnknownEnumValue), true);
final EncoderGenerator encoderGenerator = new EncoderGenerator(MESSAGE_EXAMPLE, TEST_PACKAGE,
TEST_PARENT_PACKAGE, outputManager, ValidationOn.class, RejectUnknownFieldOn.class,
Expand Down Expand Up @@ -967,6 +972,14 @@ public void shouldValidateTagNumbersDefinedForThisMessageWhenUnknownFieldPropIsS
assertEquals("Wrong reject reason", TAG_NOT_DEFINED_FOR_THIS_MESSAGE_TYPE, decoder.rejectReason());
}

@Test
public void shouldAllowMessagesWithEmptyTagsForStringPropertyWhenAllowTagsPropIsSet() throws Exception
{
final Decoder decoder = decodeHeartbeatAllowingEmptyTags(EMPTY_FIX_TAG_FOR_STRING_FIELD_MESSAGE);

assertTrue("Failed validation with empty fix tag", decoder.validate());
}

@Test
public void shouldSkipFieldUnknownToMessageButDefinedInFIXSpec() throws Exception
{
Expand Down Expand Up @@ -1927,6 +1940,13 @@ private Decoder decodeHeartbeatWithRejectingUnknownFields(final String example)
return decoder;
}

private Decoder decodeHeartbeatAllowingEmptyTags(final String example) throws Exception
{
final Decoder decoder = (Decoder)heartbeatAllowingEmptyTags.getConstructor().newInstance();
decode(example, decoder);
return decoder;
}

void decode(final String example, final Decoder decoder)
{
buffer.putAscii(1, example);
Expand Down
2 changes: 1 addition & 1 deletion ...ecs/src/test/java/uk/co/real_logic/artio/dictionary/generation/AcceptorGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public class AcceptorGeneratorTest
RejectUnknownFieldOn.class, RejectUnknownEnumValueOn.class, RUNTIME_REJECT_UNKNOWN_ENUM_VALUE_PROPERTY, true);
private static final DecoderGenerator DECODER_GENERATOR = new DecoderGenerator(
MESSAGE_EXAMPLE, 1, TEST_PACKAGE, TEST_PARENT_PACKAGE, TEST_PACKAGE, OUTPUT_MANAGER, ValidationOn.class,
RejectUnknownFieldOff.class, RejectUnknownEnumValueOn.class, false, false,
RejectUnknownFieldOff.class, RejectUnknownEnumValueOn.class, false, false, false,
Generator.RUNTIME_REJECT_UNKNOWN_ENUM_VALUE_PROPERTY, true);
private static final AcceptorGenerator ACCEPTOR_GENERATOR = new AcceptorGenerator(
MESSAGE_EXAMPLE, TEST_PACKAGE, OUTPUT_MANAGER);
Expand Down
2 changes: 1 addition & 1 deletion ...rc/test/java/uk/co/real_logic/artio/dictionary/generation/CopyToEncoderGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ private static Map<String, CharSequence> generateClasses()
final DecoderGenerator decoderGenerator = new DecoderGenerator(
MESSAGE_EXAMPLE, 1, TEST_PACKAGE,
TEST_PARENT_PACKAGE, TEST_PACKAGE, outputManager, ValidationOn.class,
RejectUnknownFieldOn.class, RejectUnknownEnumValueOn.class, false, false,
RejectUnknownFieldOn.class, RejectUnknownEnumValueOn.class, false, false, false,
RUNTIME_REJECT_UNKNOWN_ENUM_VALUE_PROPERTY, true);
final EncoderGenerator encoderGenerator = new EncoderGenerator(MESSAGE_EXAMPLE, TEST_PACKAGE,
TEST_PARENT_PACKAGE, outputManager, ValidationOn.class, RejectUnknownFieldOn.class,
Expand Down
2 changes: 1 addition & 1 deletion ...decs/src/test/java/uk/co/real_logic/artio/dictionary/generation/PrinterGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class PrinterGeneratorTest
private static final DecoderGenerator DECODER_GENERATOR = new DecoderGenerator(
MESSAGE_EXAMPLE, 1, TEST_PACKAGE, TEST_PARENT_PACKAGE, TEST_PACKAGE,
OUTPUT_MANAGER, ValidationOn.class,
RejectUnknownFieldOff.class, RejectUnknownEnumValueOn.class, false, false,
RejectUnknownFieldOff.class, RejectUnknownEnumValueOn.class, false, false, false,
Generator.RUNTIME_REJECT_UNKNOWN_ENUM_VALUE_PROPERTY, true);
private static final EncoderGenerator ENCODER_GENERATOR = new EncoderGenerator(
MESSAGE_EXAMPLE, TEST_PACKAGE, TEST_PARENT_PACKAGE, OUTPUT_MANAGER, ValidationOn.class,
Expand Down
2 changes: 1 addition & 1 deletion ...test/java/uk/co/real_logic/artio/dictionary/generation/ToEncoderDecoderGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ private static Map<String, CharSequence> generateClasses(final boolean flyweight
final DecoderGenerator decoderGenerator = new DecoderGenerator(
MESSAGE_EXAMPLE, 1, TEST_PACKAGE,
TEST_PARENT_PACKAGE, TEST_PACKAGE, outputManager, ValidationOn.class,
RejectUnknownFieldOn.class, RejectUnknownEnumValueOn.class, flyweightStringsEnabled, false,
RejectUnknownFieldOn.class, RejectUnknownEnumValueOn.class, flyweightStringsEnabled, false, false,
RUNTIME_REJECT_UNKNOWN_ENUM_VALUE_PROPERTY, true);
final EncoderGenerator encoderGenerator = new EncoderGenerator(MESSAGE_EXAMPLE, TEST_PACKAGE,
TEST_PARENT_PACKAGE, outputManager, ValidationOn.class, RejectUnknownFieldOn.class,
Expand Down
Loading