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

[MNG-8024] Make WrapperProperties and WrapperList serizalizable. #1388

Closed
wants to merge 2 commits into from

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Jan 21, 2024

This solves the two, but there are more...


https://issues.apache.org/jira/browse/MNG-8024

@cstamas cstamas self-assigned this Jan 21, 2024
@gnodet
Copy link
Contributor

gnodet commented Jan 22, 2024

We need to extend the unit test https://github.com/apache/maven/blob/master/maven-model/src/test/java/org/apache/maven/model/SerializationTest.java to exercise the actual problem.

@@ -27,16 +27,21 @@
import java.util.function.Supplier;

class WrapperList<T, U> extends AbstractList<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point is the WrapperList itself does not implement Serializable ?

Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a clear unit test. Basic serialisation is already supported, so something's wrong...
The WrapperList and WrapperProperties should not actually be used during serialisation, as those objects are not present in fields.

@gnodet
Copy link
Contributor

gnodet commented Jan 22, 2024

We need a clear unit test. Basic serialisation is already supported, so something's wrong... The WrapperList and WrapperProperties should not actually be used during serialisation, as those objects are not present in fields.

I wonder if the problem is when serializing part of the model instead of the whole Model.

@cstamas
Copy link
Member Author

cstamas commented Jan 23, 2024

Yes, that is exactly what is happening, the incremental build basically taks "bits" of model and would like to serialize those with DataOutputStream...

@cstamas cstamas marked this pull request as draft January 23, 2024 12:00
@gnodet
Copy link
Contributor

gnodet commented Jan 23, 2024

Yes, that is exactly what is happening, the incremental build basically taks "bits" of model and would like to serialize those with DataOutputStream...

I'm not sure we should support this use case, as I think the whole model will be serialised anyway. The lambdas in the list/properties objects do retain a reference to the parent up to the Model. This is needed in order to implement the modification methods while keeping the original reference.

So while this may actually make things work, I'm not completely sure we should do it... It may be easier to simplify check if there's a getDelegate() method, and if so, serialize that object instead.

@gnodet
Copy link
Contributor

gnodet commented Jan 23, 2024

Yes, that is exactly what is happening, the incremental build basically taks "bits" of model and would like to serialize those with DataOutputStream...

I'm not sure we should support this use case, as I think the whole model will be serialised anyway. The lambdas in the list/properties objects do retain a reference to the parent up to the Model. This is needed in order to implement the modification methods while keeping the original reference.

So while this may actually make things work, I'm not completely sure we should do it... It may be easier to simplify check if there's a getDelegate() method, and if so, serialize that object instead.

Another way would be to generate writeObject / readObject methods that will actually delegate to... the delegates from the v4 model.

@gnodet
Copy link
Contributor

gnodet commented Mar 1, 2024

Superseded by #1433

@gnodet gnodet closed this Mar 1, 2024
@cstamas cstamas deleted the MNG-8024 branch March 1, 2024 17:30
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