-
Notifications
You must be signed in to change notification settings - Fork 81
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
[WFLY-13412]:Add_Json_Merge_Patch_support #367
Conversation
|
||
=== Hard Requirements | ||
|
||
* RESTEasy includes this feature support and is installed in WFLY/EAP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This analysis should include the fact that we need to backport the feature as per https://issues.redhat.com/browse/RESTEASY-2567 since now it reads that no work is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already included in 3.15.9.Final. There is no work is needed for EAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimma My bad, I see the test now in 3.15 branch https://github.com/resteasy/resteasy/blob/3.15/testsuite/integration-tests/src/test/java/org/jboss/resteasy/test/resource/patch/StudentPatchTest.java So the https://issues.redhat.com/browse/RESTEASY-2567 is outdated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my mistake. Checked it again and json merge patch isn't backported in 3.15.x, but this is already included since 4.1.x , should be already included in EAP8 and no work is need for EAP8.
cf1d5a6
to
ed71ee2
Compare
@jimma the analysis doesn't include the sections regarding configuration and deployment, I am assuming this means that this new feature has no side effects on old server configs, and require no changes to be done by customers in their apps, when they migrate... Can you please confirm my analysis of zero migration impact is correct? |
|
||
== Requirement | ||
|
||
=== Hard Requirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@honza-kasik It looks Resteasy AbstractPatchMethodFilter handles this correctly. By default, it uses Jakarta JSON Processing PATCH filter to handle json patch and json merge patch request. The Jackson PATCH filter is only enabled when user sets the RESTEASY_PATCH_FILTER_LEGACY parameter to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jimma ! Can this mention about the parameter be added to this document please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@honza-kasik I already added to this document, please have a look.
@emmartins Yes, I confirmed and there is no migration impact. |
@jimma got it, thanks |
9f1fcb0
to
3df60ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good from QE PoV
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be migrated to the new format defined in https://github.com/wildfly/wildfly-proposals/blob/main/design-doc-template.adoc?plain=1.
It will also need to follow the new process defined in https://github.com/wildfly/wildfly-proposals/blob/main/FEATURE_PROCESS.adoc.
@jamezp Updated this PR. Thanks for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should ensure all the references to JSON are referenced as "JSON" all caps. We should be referencing RESTEasy in all cases too. There is currently a mix of cases for RESTEasy.
Resteasy provides two implementation for JSON merge patch. One based on jsonp(abbreviation jsonp will be used in this page) and another one is based on | ||
json-patch and jackson2(abbreviation jackson2 will be used in this page). By default, Resteasy uses jsonp implementation, while the jackson2 implementation is only enabled | ||
when the context parameter `resteasy.patchfilter.legacy=true` is set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only discuss the Jakarta JSON Processing option IMO. We shouldn't mention the legacy option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is requested by @honza-kasik, as this is still there and still can be set to switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about these additional changes. I wasn't able to locally render this site with Jekyll before and I was finally able to figure out how to get it to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed the file name includes "EAP7-1494", that should be removed. It should also probably be renamed to something like WFLY-19981_Json_Merge_Patch_suppoert.adoc
.
@jamezp Change are squashed. Please review and merge. Thanks. |
No description provided.