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

(FFM-4463) Update parser validation #39

Merged
merged 1 commit into from Sep 1, 2022

Conversation

conormurray95
Copy link
Contributor

@conormurray95 conormurray95 commented Sep 1, 2022

Issue
Tylertech experienced an issue using the .NET sdk with ff-proxy where if they had a target group with no rules configured the .NET sdk would fail to parse the target groups from the http response during initialisation and error out.
We also previously had this issue from Sharp Gaming if the feature variationToTargetMap was empty.

After digging into this issue I noticed that both of these problem fields were the only properties in our generated spec annotated with AllowNull instead of DisallowNull . The behaviour of these two properties are described here:
AllowNull: The property must be defined in JSON but can be a null value.
DisallowNull: The property is not required but it cannot be a null value.
Hence the reason it failed to parse was because the proxy didn't send these properties if they didn't exist (which is the correct behaviour)
Default: The property is not required. The default state.

Solution
My plan was to install nswag and play around with the code generation properties until I could get these two fields to also generate as DisallowNull fields.
However after just running nswag with the current config I noticed it auto changed them to DisallowNull fields anyway, so they must have been manually edited when these generated files were first committed.
After changing them to DisallowNull though I realised that the build tests were failing because they contain a null variationToTargetMap here (I assume this is why it was manually changed in the first place so these tests could pass)

The less than ideal (but better than what we have now) solution I've gone for is to change these fields to have the property Default which allows the fields to be either null or missing. Ideally all fields in this generated spec should be like this but after searching github discussions there doesn't seem to be any easy way to do it bar manually changing the generated code.

Testing
Tested against ff-proxy and it solves the issue the customer was seeing
Tested against saas and behaviour is still correct

Longer term
We should have a way to generate the annotations (or lack of) that satisfies the needs of the sdk and also the test cases, from looking through the spec it appears there were a few other manual edits that stray from the originally generated code, this could cause problems if we ever need to rev this spec

@swarmia
Copy link

swarmia bot commented Sep 1, 2022

✅  Linked to Bug FFM-4463 · Proxy offline mode issue loading config

@conormurray95 conormurray95 force-pushed the FFM-4463-update-parser-validation branch 2 times, most recently from db7d18b to 8ab52f0 Compare September 1, 2022 13:36
@conormurray95 conormurray95 force-pushed the FFM-4463-update-parser-validation branch from 8ab52f0 to 360b0e2 Compare September 1, 2022 13:55
@conormurray95 conormurray95 merged commit 1ff56eb into main Sep 1, 2022
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.

None yet

2 participants