Skip to content

Added some tests for PatchBuilder replaceInsert #280

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

Closed
wants to merge 2 commits into from

Conversation

nikos
Copy link
Contributor

@nikos nikos commented Mar 30, 2015

With the current situation of the b3_0 sources, it looks like there are no tests for the straight replaceInsert functionality of the patch builder (EDIT: there seems to be one test under test-complete directory called TestPartialUpdate which replace/inserts on array elements). In a recent project I found out, that the behavior of the patch builder does differ if the node does exist (replace) vs. if does not yet exist (insert: will put the whole fragment instead of the selectPath). This behavior is for the application developer a bit inconvenient, since you have to check first wether the node does exist or not, and then to decide how you structure the fragment node in question.

NOTE: Therefore this test case highlights the current problem and the second assertion (L. 357, "please make me not nested") will fail.

With the current situation of the b3_0 sources, it looks like there are no tests for the replaceInsert functionality of the patch builder. In a recent project I found out, that the behavior of the patch builder does differ if the node does exist (replace) vs. if does not yet exist (insert: will put the whole fragment instead of the selectPath). This behavior is for the application developer a bit inconvenient, since you have to check first wether the node does exist or not, and then to decide how you structure the fragment node in question.

NOTE: Therefore this test case highlights the current problem and the second assertion (L. 357, "please make me not nested") will fail.
@nikos
Copy link
Contributor Author

nikos commented Mar 31, 2015

Thanks @sammefford for bringing up the imports, I did fix those with 245a798
And yes, I did work on the b3_0 branch, for future PRs I will switch to the release-3.0.1 branch then.

@sammefford
Copy link
Contributor

Actually, the working development branch is 'develop'. That's the one to use. I can always back-port to other versions you specify where the fix is also needed. We don't usually start work on a maintenance branch then push the changes to develop.

@sammefford
Copy link
Contributor

Also, as I mentioned on #280, based on the email discussion thread yesterday which I summarized, it sounds like you were able to get an answer on why the behavior is the way it is currently, and a viable workaround. Is there any need for this pull request still?

@nikos
Copy link
Contributor Author

nikos commented Mar 31, 2015

In my opinion it would be great if the current behavior of replace-insert for patching a property on top-level in JSON documents is documented as a test case. If in a later release the behavior will be changed, we would then of course adapt the test case as well.

@sammefford
Copy link
Contributor

If I understand correctly, you're recommending we add an example to show the recommended approach for this use case. We'll do that in the REST docs since it is the same principles for REST, Java, and Node.js. So I'll close this issue and #280 WRT Java. If you'd like to track progress on the docs change, please file a docs bug in bugtrack.

@sammefford sammefford closed this Mar 31, 2015
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.

2 participants