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

JsonPath transformation service comply with the contract #4379

Merged
merged 1 commit into from Oct 10, 2017

Conversation

Projects
None yet
4 participants
@sjka
Copy link
Contributor

commented Oct 5, 2017

...as it was returning null in case the path pointed to an element which did not exist.
Now it throws a TransformationException.

I'm not 100% sure though if this really is always the right way of fixing it. A missing element in JSON is somewhat similar to "intentionally null", i.e. UndefType.UNDEF might also be a consideration. Any deeper insights anybody?

fixes #4376
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com

JsonPath transformation service comply with the contract
...as it was returning null in case the path pointed to an element which did not exist.

fixes #4376
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

@sjka sjka force-pushed the sjka:fixJsonTransformation branch from 5cd0969 to f90852c Oct 6, 2017

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Oct 6, 2017

Any deeper insights anybody?

Not from my end.
Shall we wait for insights from others or "just merge"...?

@sjka

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2017

Then suggest using merging it because it now fits to the javadoc contract. If it turns out this is not appropriate, then we still can adapt it afterwards.

@martinvw

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

Hmm, reading your suggestion UndefType.UNDEF or UndefType.NULL might make more sense, it is after all an element which may be only missing this time. And it's not uncommon in JSON the leave out any null values, see for example some NPE's I got in the Nest binding because I do not posses all Nest devices.

@htreu

htreu approved these changes Oct 9, 2017

Copy link
Contributor

left a comment

lgtm, should be merged.

@htreu

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2017

@martinvw imho the transformation service is right about throwing a TransformationException. Its in the responsibility of the caller to act appropriate. And from the code I saw the exception is handled well by all callers.

@kaikreuzer kaikreuzer merged commit f1c63ba into eclipse:master Oct 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details

@sjka sjka deleted the sjka:fixJsonTransformation branch Nov 14, 2017

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.