jsfaure commented 4 years ago

Problem

'FAIL_ON_UNKNOWN_PROPERTIES' Deserialisation Feature doesn't work as expected when the JSON contains less fields than the CaseClass.

In CaseClassDeserialiazer, the sequence of unknown fields is taken into account only when the number of fields provided in the Json is greater than the number of fields declared in the CaseClass. For instance, when the case class contain some optional fields, the JSON can contain less fields then the case class and one of them can be unknown.

Solution

The solution is to always take into account unknown properties whatever maybe the number of fields.

Result

When the JSON contains less fields overall than the CaseClass, When the JSON provide one extra unknown field, When the FAIL_ON_UNKNOWN_PROPERTIES is on, Then an exception is thrown.

CLAassistant commented 4 years ago

All committers have signed the CLA.

codecov[bot] commented 4 years ago

Codecov Report

Merging #544 into develop will increase coverage by 0.08%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #544      +/-   ##
===========================================
+ Coverage    90.59%   90.68%   +0.08%     
===========================================
  Files          267      267              
  Lines         4712     4779      +67     
  Branches       292      305      +13     
===========================================
+ Hits          4269     4334      +65     
- Misses         443      445       +2

Impacted Files	Coverage Δ
...atra/jackson/caseclass/CaseClassDeserializer.scala	`95.27% <100.00%> (+0.05%)`	:arrow_up:
...cala/com/twitter/inject/server/TwitterServer.scala	`82.43% <0.00%> (-1.13%)`	:arrow_down:
.../finatra/jackson/caseclass/InjectableValueId.scala	`84.61% <0.00%> (-1.10%)`	:arrow_down:
...n/scala/com/twitter/finatra/json/JsonLogging.scala	`100.00% <0.00%> (ø)`
...om/twitter/finatra/jackson/ScalaObjectMapper.scala	`100.00% <0.00%> (ø)`
...om/twitter/finatra/json/utils/JsonDiffResult.scala	`100.00% <0.00%> (ø)`
...http/internal/routing/RequestWithRouteParams.scala	`100.00% <0.00%> (ø)`
...ra/jackson/caseclass/DefaultInjectableValues.scala	`100.00% <0.00%> (ø)`
...inatra/http/benchmark/FinagleBenchmarkServer.scala	`100.00% <0.00%> (ø)`
...eptions/DetailedNonRetryableSourcedException.scala	`100.00% <0.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3eb0cd8...ecbc0d7. Read the comment docs.

cacoco commented 4 years ago

@jsfaure thanks for the review! Can you see what's up with the TravisCI builds here?

jsfaure commented 4 years ago

Hi @cacoco

@jsfaure thanks for the review! Can you see what's up with the TravisCI builds here?

I can't say what happened. The CI went well 2 times previously. I updated the PR (only comments) to trigger new builds and they get stucked (received but not even started) and I can't cancel them.

However, the build works fine from our fork: https://travis-ci.com/github/criteo-forks/finatra/builds/176632369

Coming back to the change itself, Even though the code change won't break any compilation, I marked it with 'API BREAKING CHANGE' in the changelog because it may break integration between clients and server in a REST API context.

Please, note that this update will make it comply with the Java Jackson library.

ryanoneill commented 4 years ago

Looks like the issues with travis have resolved themselves. I'm going to pull this in internally and try to merge it into our codebase. When that happens, it'll be pushed back out to GitHub with your attribution and credit intact. Thanks for the contribution. I'll follow up here if we run into issues or to let you know that it's been merged.

ryanoneill commented 4 years ago

Just fyi, this change is going to require some updates to be able to be merged internally. I'm checking into the details now.

ryanoneill commented 4 years ago

It appears that at least one team is depending on the prior behavior, so we'll need to deal with that.

jsfaure commented 4 years ago

Thank you for your feedback. Since this change can break current integrations, we could consider adding a new Flag, we can call it something like "FAIL_ON_UNKNOWN_PROPERTIES_BACKWARD_COMPATIBLE". The new behavior could be enabled by default and the new flag would let people restore the previous behavior when switched on. When both the feature FAIL_ON_UNKNOWN_PROPERTIES and the new flag are on, a warning could be emitted to remind people that this is not a recommended behavior. When the new flag is off, another warning could be emitted to warn people that we changed the behavior and point to the change log. (The new behavior could also be disabled by default but I would not recommend this as the behavior is not expected.) Let me know how do you think.

cacoco commented 3 years ago

@jsfaure I believe I have a patch to fix this properly which also deals with the internal users. Hope to get it merged and out soon. Thanks!

jsfaure commented 3 years ago

@cacoco , thank you guys !

twitter / finatra

finatra-jackson: Fix unknown properties handling. #544

Codecov Report