QFJ-981 / QFJ-982 Remove settings from `DataDictionary` class

[philipwhiuk]

Not much testing on this but here's where we're/I'm thinking of going with it.

[chrjohn]

Did not follow this up through the changed code yet but there are NullPointerExceptions at

at quickfix.DataDictionary.checkHasValue(DataDictionary.java:683)
at quickfix.DataDictionary.iterate(DataDictionary.java:555)
at quickfix.DataDictionary.validate(DataDictionary.java:538)
at quickfix.Session.next(Session.java:998)

Probably forgotten to pass the default validation settings?!

Thanks for the PR!

[chrjohn]

Hi @philipwhiuk , I wanted to add a test for QFJ-981 which is related to this issue. However, I do not seem to be able to push changes to this branch. Have you enabled push access for maintainers on this PR? Thanks, Chris.

[philipwhiuk]

Yep I allowed edits from maintainers... Not sure why it won't let you push..

[chrjohn]

Got a 'permission denied'. Maybe got one of the parameters wrong. Will try again later.

[chrjohn]

Nevermind, turned out I was using the wrong branch

[philipwhiuk]

I'm wondering whether it's better to rename the new struct to ValidationSettings. It's kind of a half way between SessionSettings and DataDictionarySettings.

[chrjohn]

I'm wondering whether it's better to rename the new struct to ValidationSettings. It's kind of a half way between SessionSettings and DataDictionarySettings.

Sounds sensible. ValidationSettings is more descriptive.

[philipwhiuk]

While trying to test this, I tried to design scenario that was easy to QA. I ran into #247 which I didn't expect.

I'm trying to decide whether my calling code should call DataDictionary.validate after constructing a message or whether #247 is valid.

[chrjohn]

Hi @philipwhiuk , I was thinking about into which version this fix needs to go. When using semantic versioning (which I'm roughly trying to follow) this needs to go into 3.0 due to public interface changes, right?

[chrjohn]

I think I failed at trying to resolve the merge conflict. Will check.

[philipwhiuk]

Hi @philipwhiuk , I was thinking about into which version this fix needs to go. When using semantic versioning (which I'm roughly trying to follow) this needs to go into 3.0 due to public interface changes, right?

Yes, it's probably 3.0. We're testing this change internally at current. No issues so far :)

[the-thing]

@chrjohn

QFJ-981/QFJ-982 are actually a duplicate of 2016 issue QFJ-877, but there was no investigation so it can be closed once this is merged.

@philipwhiuk

You probably need to add null checks against quickfix.ValidationSettings inside quickfix.Message class (possibly in quickfix.DataDictionary as well). The code assumes that data dictionary and validation settings are either both null or or none. However, these are public methods and they can be called with any arguments.

• quickfix.DataDictionary#validate(quickfix.Message, quickfix.ValidationSettings)
• quickfix.Message#fromString(java.lang.String, quickfix.DataDictionary, quickfix.DataDictionary, quickfix.ValidationSettings, boolean, boolean)

Additionally, these methods were not overloaded. This is a breaking change as I have seen these methods being used in test and FIX simulator code while working for at least two clients. This one might not be a big deal and probably does not need to be fixed, but it will add some work for developers integrating with the newest version.

[philipwhiuk]

Yeah it's a breaking change. I think the plan is to release this as 3.0.0

I'm happy to review PRs to add null checks.

We're using this internally and it works fine.

[the-thing]

Null checks would be good to have, but your choice. I would not worry to much about it.