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.

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

Allow disabling validation on empty tags #497

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

Conversation

writeoncereadmany
Copy link

We are encountering situations where we're being sent FIX messages with empty tags (eg |448=|). Currently, this fails validation.

Whilst this is clearly invalid FIX, we can't legislate for what our counterparties send us: we've had multiple counterparties send us malformed FIX in this way. It would be nice to be able to, instead of failing on such messages, instead to treat these tags as missing (still triggering required fields validation if the field is required).

This change replicates the approach taken to other settings allowing enabling/disabling of validations, and includes a minimal test to show that the messages pass validation with absent fields when this specific validation is disabled.

… empty tags on decoders, and treat them as missing tags if encountered when validation is off.
…pply empty tag validation to a codec build-time property. note: no mechanism for setting this build-time property has been added yet.
…system property passed in to the codec generator
…rs under JDK21. There are ways of solving this without the suppression, but they require more discussion and work: this is a good starting point for what's likely to be over-eager analysis for most use cases.
@writeoncereadmany
Copy link
Author

note: CI keeps failing on:

uk.co.real_logic.artio.system_tests.TestRequestOutOfSequenceShouldNotTriggerHeartbeatBeforeResendRequestTest

This test seems wildly intermittent at the moment, and is also failing on this test on master - so I would argue this shouldn't be a blocker to merging? However: we're not getting any CI failures that can be attributed to this change, as far as I can see.

@tom-smalls
Copy link
Contributor

Is this going to get merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants