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

Added AllowUnknownFieldValues and AllowUnknownMsgFields configuration… #543

Closed
wants to merge 1 commit into from

Conversation

peto268
Copy link
Contributor

@peto268 peto268 commented Aug 20, 2019

… settings.

This should resolve #430

@CLAassistant
Copy link

CLAassistant commented Aug 20, 2019

CLA assistant check
All committers have signed the CLA.

@peto268
Copy link
Contributor Author

peto268 commented Sep 6, 2019

Added one more exclude to the CheckValidTagNumber method so it wont throw an exception even for fields that aren't even defined in the dictionary.

@mbmurray
Copy link

Has this pull request been abandoned?

@gbirchmeier
Copy link
Member

gbirchmeier commented Sep 24, 2019 via email

gbirchmeier added a commit to gbirchmeier/quickfixn that referenced this pull request Mar 2, 2020
Added by connamara#543 submission, but there's no need or
precedent for it.
gbirchmeier added a commit to gbirchmeier/quickfixn that referenced this pull request Mar 3, 2020
gbirchmeier added a commit to gbirchmeier/quickfixn that referenced this pull request Mar 3, 2020
...because that would break group parsing.
(Parser would not be able to know when the group
ends!)

 connamara#430 connamara#543
gbirchmeier added a commit to gbirchmeier/quickfixn that referenced this pull request Mar 3, 2020
@gbirchmeier
Copy link
Member

I adapted this fix in #577.

I removed support for AllowUnknownFieldValues, because there's no precedent for it and it's not really needed. (The issues it addresses could be solved by simply editing the DD.)

@gbirchmeier gbirchmeier closed this Mar 3, 2020
@roblugt
Copy link
Contributor

roblugt commented May 24, 2020

I would like to add my voice to the request to re-add support for AllowUnknownFieldValues. Whilst its true that a similar result could be achieved by modifying the DataDictionary to remove the enums completely, this overlooks the fact that applications may be using the dictionary for other pruposes - and it is useful to know if a field value falls into the known set or is for an unknown value. The DD is also useful for documenting the expected values (but still acknowledging that unknown values can be accepted)

@peto268
Copy link
Contributor Author

peto268 commented May 25, 2020

The reason why I've added it in the first place is that when you are integrating with a party that tends to use a lot of customization on top of vanilla FIX spec, you can end up doing a lot of DD tweaks just to make quickfixn happy and not fail on deserialisation. Even for fields / values that you don't really care about. Yes, it might be a precedent, but a useful one.

@roblugt
Copy link
Contributor

roblugt commented May 26, 2020 via email

@gbirchmeier
Copy link
Member

New features add code complexity and can contribute to future maintenance, so I do tend to scrutinize them and avoid ones that don't seem crucial. Precedent by C++ and Java makes the case stronger, since we don't want the ports to differ greatly.

You have made a decent case, though. Let me bounce it off my colleagues.

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.

Support validation configuration "AllowUnknownMsgFields"
5 participants