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

Update PersistentCollectionSerializer.java #173

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

Taha-Di-Nero
Copy link
Contributor

fix REPLACE_PERSISTENT_COLLECTIONS in EAGER #146

fix REPLACE_PERSISTENT_COLLECTIONS in EAGER FasterXML#146
@pjfanning
Copy link
Member

pjfanning commented May 2, 2023

@cowtowncoder I am not a lawyer - but this PR seems to be a direct copy of #146 (issue #140) so this PR should probably not be considered the original work of @Taha-Di-Nero - this makes it problematic to merge this PR since the copyright on the code here is at least debatable.

@cowtowncoder
Copy link
Member

cowtowncoder commented May 3, 2023

We'd also need this for all backends, not just H5 and H6.

As to contribution, this is not a super sizable change so one could argue that one could come up with this on their own (there aren't that many ways to do it).
But yeah, probably was just copied.

Let's hope author of #146 can contribute it, if so this could be just addition to H6.

@Taha-Di-Nero
Copy link
Contributor Author

Taha-Di-Nero commented May 4, 2023

@pjfanning @cowtowncoder Yes this is applying the same patch done in #146 but for hibernate 6, the author of #146 is my colleague and I asked him to do what @cowtowncoder requested but he doesn't want to do it.
We need only hibernate 6 and we manage it via a small fork that we would like to remove, so please merge it.
Best regards

@cowtowncoder
Copy link
Member

@Taha-Di-Nero Ok as long as author is fine with your contributing the code (you have discussed it), and you are ok sending CLA (which just attests that you think you can contribute the code), I think we can merge it.

But one thing I realize now is that there is no unit test to verify this fix works. I guess you are using a fork so I don't doubt it doesn't, but if there was one way to test it that'd obviously be great.

@cowtowncoder cowtowncoder added the cla-needed Requires CLA before merging label May 16, 2023
@Taha-Di-Nero
Copy link
Contributor Author

Taha-Di-Nero commented May 24, 2023

@cowtowncoder contributor-agreement.pdf
Hi
Unfortunately we didn't implemented any tests.
Best regards

@cowtowncoder cowtowncoder removed the cla-needed Requires CLA before merging label May 26, 2023
@cowtowncoder
Copy link
Member

cowtowncoder commented May 26, 2023

Excellent, thank you @Taha-Di-Nero ! Ok, aside from tests (it's ok since you have tested this), could I ask same changes for hibernate5 and hibernate5-jakarta modules? :)

If you don't have time I can do it later on but just in case. I hope to merge this later today -- thank you once again for contributing this. It will make it in 2.16.0.

@cowtowncoder cowtowncoder merged commit 6b1bb45 into FasterXML:master Jun 6, 2023
1 check passed
@Taha-Di-Nero Taha-Di-Nero deleted the patch-1 branch June 13, 2023 08:53
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

3 participants