Xstream keeps path references to unreferenceable objects #3

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@Groostav

Groostav commented May 5, 2015

continuation of The PR over at gg-xstream

@joehni My apologies for all the trouble! This is my first contribution to a significant open source project, and my first attempt at cross-project github use.

@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav May 5, 2015

I didn't do anything clever regarding branching to make this pull request, I just bought up my version of the code-base on one screen, this branch on another, and the diffs on a third, and then started working through the diffs one by one.

If I can put together the time, I'd like to figure out how to set my fork's upstream to the work I did on the other repo, and then try a git pull.

If nothing else it would make for one hell of an interesting merge in my fancy new 'Semantic Merge' merge tool!

Groostav commented May 5, 2015

I didn't do anything clever regarding branching to make this pull request, I just bought up my version of the code-base on one screen, this branch on another, and the diffs on a third, and then started working through the diffs one by one.

If I can put together the time, I'd like to figure out how to set my fork's upstream to the work I did on the other repo, and then try a git pull.

If nothing else it would make for one hell of an interesting merge in my fancy new 'Semantic Merge' merge tool!

+ * Whether this type is an immutable type whose references must be kept anyways
+ * (for compatibility) at deserialization.
+ *
+ * @since 1.4.9

This comment has been minimized.

@Groostav

Groostav May 5, 2015

not sure what to do about this, do you guys enforce @since tags, and whats the correct value?

@Groostav

Groostav May 5, 2015

not sure what to do about this, do you guys enforce @since tags, and whats the correct value?

+// fail("Thrown " + CannotResolveClassException.class.getName() + " expected");
+// } catch (final CannotResolveClassException e) {
+// assertEquals(name, e.getMessage());
+// }

This comment has been minimized.

@Groostav

Groostav May 5, 2015

this was failing as of checkout on my branch, I'll revert this now.

@Groostav

Groostav May 5, 2015

this was failing as of checkout on my branch, I'll revert this now.

* @throws InitializationException if no {@link ImmutableTypesMapper} is available
+ * @throws IllegalArgumentException if <code>type</code> is <tt>null</tt>
+ * @since 1.4.9

This comment has been minimized.

@Groostav

Groostav May 5, 2015

here also, this is presumably 1.5.something

@Groostav

Groostav May 5, 2015

here also, this is presumably 1.5.something

@joehni

This comment has been minimized.

Show comment
Hide comment
@joehni

joehni May 5, 2015

Member

Hi Geoff,

well, actually you have to bear with me too, I did not really use GitHub myself until two weeks ago.

Concerning the changes - it looks a lot better now! I was really irritated by the original diff containing this mix of Java 5 features and code that did not support it.

Minor stuff (incl. some unnecessary changes):

  • the method and variable names canBeReferencedByPath and parameter name retainPathsOnDeserialization are sematically wrong. The reference mechanism works alternatively with identifiers or even custom references. Please us Mapper.isReferenceable and according variable names
  • DefaultMapper: The default for isReferenceable has to be true
  • AbstractReferenceUnmarshaller: the algorithmus should be - if reference is not null, check if the type is referenceable (immutability does not matter here). If not, throw ConverterException("Unrefernceable type") else continue to get the cached value
  • ImmutableTypesMapper: Parameters are suddenly no longer final and the generic declaration is missing
  • add @SInCE with upcoming as value instead of a version number
  • no diamond operator, it is not Java 6 compatible
  • no resort of the import statements, please
  • no change of existing variable names
  • no parameter alignments (boolean arguments in setupImmutableTypes())
  • no change of visibility for variables for the sake of tests, because it changes the API. If you really have to test it this way, please use reflection to access the fields. Another way to ensure, that no reference is kept is to unmarshal XML with a reference and expect XStream to fail.
  • change of return type in ReferenceByXPathMarshallingStrategy and in XStream12CompatibilityTest is unrelated

Thanks for your time!

Member

joehni commented May 5, 2015

Hi Geoff,

well, actually you have to bear with me too, I did not really use GitHub myself until two weeks ago.

Concerning the changes - it looks a lot better now! I was really irritated by the original diff containing this mix of Java 5 features and code that did not support it.

Minor stuff (incl. some unnecessary changes):

  • the method and variable names canBeReferencedByPath and parameter name retainPathsOnDeserialization are sematically wrong. The reference mechanism works alternatively with identifiers or even custom references. Please us Mapper.isReferenceable and according variable names
  • DefaultMapper: The default for isReferenceable has to be true
  • AbstractReferenceUnmarshaller: the algorithmus should be - if reference is not null, check if the type is referenceable (immutability does not matter here). If not, throw ConverterException("Unrefernceable type") else continue to get the cached value
  • ImmutableTypesMapper: Parameters are suddenly no longer final and the generic declaration is missing
  • add @SInCE with upcoming as value instead of a version number
  • no diamond operator, it is not Java 6 compatible
  • no resort of the import statements, please
  • no change of existing variable names
  • no parameter alignments (boolean arguments in setupImmutableTypes())
  • no change of visibility for variables for the sake of tests, because it changes the API. If you really have to test it this way, please use reflection to access the fields. Another way to ensure, that no reference is kept is to unmarshal XML with a reference and expect XStream to fail.
  • change of return type in ReferenceByXPathMarshallingStrategy and in XStream12CompatibilityTest is unrelated

Thanks for your time!

@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav May 7, 2015

  • the method and variable names canBeReferencedByPath and parameter name retainPathsOnDeserialization are sematically wrong. The reference mechanism works alternatively with identifiers or even custom references. Please us Mapper.isReferenceable and according variable names --I was intentionally avoiding this name because isReferenceable is a tataulogy; what type can't be referenced? I will go forward with this name change but I'm wondering if theres something less ambiguous we can do. Is there any location (eg a glossary) in XStream that I can add the term isReferenceable?
  • DefaultMapper: The default for isReferenceable has to be true --will do
  • AbstractReferenceUnmarshaller: the algorithmus should be - if reference is not null, check if the type is referenceable (immutability does not matter here). If not, throw ConverterException("Unrefernceable type") else continue to get the cached value --I'll take this to mean my comment about subtypes and references is too obscure, will fix.
  • ImmutableTypesMapper: Parameters are suddenly no longer final and the generic declaration is missing --will fix
  • add @SInCE with upcoming as value instead of a version number --will fix
  • no diamond operator, it is not Java 6 compatible --will fix
  • no resort of the import statements, please --This is my IDE being annoying, will fix
  • no change of existing variable names --I really don't like the name values, but I will revert my change.
  • no parameter alignments (boolean arguments in setupImmutableTypes()) --will fix
  • no change of visibility for variables for the sake of tests, because it changes the API. If you really have to test it this way, please use reflection to access the fields. Another way to ensure, that no reference is kept is to unmarshal XML with a reference and expect XStream to fail. --Considering the switch you're asking me to make above the exception I'll be raising might be enough, but in the success cases I'm not sure it will be, I can use reflection to get at these fields, but how can changing a field from private to package-protected break binary compatibility?
  • change of return type in ReferenceByXPathMarshallingStrategy and in XStream12CompatibilityTest is unrelated --I needed this change to avoid a cast in a test, I cant remember where specifically. Will fix.

Groostav commented May 7, 2015

  • the method and variable names canBeReferencedByPath and parameter name retainPathsOnDeserialization are sematically wrong. The reference mechanism works alternatively with identifiers or even custom references. Please us Mapper.isReferenceable and according variable names --I was intentionally avoiding this name because isReferenceable is a tataulogy; what type can't be referenced? I will go forward with this name change but I'm wondering if theres something less ambiguous we can do. Is there any location (eg a glossary) in XStream that I can add the term isReferenceable?
  • DefaultMapper: The default for isReferenceable has to be true --will do
  • AbstractReferenceUnmarshaller: the algorithmus should be - if reference is not null, check if the type is referenceable (immutability does not matter here). If not, throw ConverterException("Unrefernceable type") else continue to get the cached value --I'll take this to mean my comment about subtypes and references is too obscure, will fix.
  • ImmutableTypesMapper: Parameters are suddenly no longer final and the generic declaration is missing --will fix
  • add @SInCE with upcoming as value instead of a version number --will fix
  • no diamond operator, it is not Java 6 compatible --will fix
  • no resort of the import statements, please --This is my IDE being annoying, will fix
  • no change of existing variable names --I really don't like the name values, but I will revert my change.
  • no parameter alignments (boolean arguments in setupImmutableTypes()) --will fix
  • no change of visibility for variables for the sake of tests, because it changes the API. If you really have to test it this way, please use reflection to access the fields. Another way to ensure, that no reference is kept is to unmarshal XML with a reference and expect XStream to fail. --Considering the switch you're asking me to make above the exception I'll be raising might be enough, but in the success cases I'm not sure it will be, I can use reflection to get at these fields, but how can changing a field from private to package-protected break binary compatibility?
  • change of return type in ReferenceByXPathMarshallingStrategy and in XStream12CompatibilityTest is unrelated --I needed this change to avoid a cast in a test, I cant remember where specifically. Will fix.
@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav May 7, 2015

also, referenceable or referentiable?

Groostav commented May 7, 2015

also, referenceable or referentiable?

@joehni

This comment has been minimized.

Show comment
Hide comment
@joehni

joehni May 7, 2015

Member

What type can't be referenced? - Wiktionary defines referenceable (see link above) as: Capable of being referenced. That's exactly what happens. Setting referenceable to false for a type ha sthe effect that you cannot have references in the XML to an object of such a type. I see no tautology here, but I am no native speaker.

Change of visibility: I did not say that it breaks binary compatibility, I said it changes the API. While package-private is not that different, we don't ajdust visibility for the sake of tests. If someone accesses the field using relfection, he knows (well, he should) that he cannot rely on binary compatibility anymore.

Thanks for your patience.

Member

joehni commented May 7, 2015

What type can't be referenced? - Wiktionary defines referenceable (see link above) as: Capable of being referenced. That's exactly what happens. Setting referenceable to false for a type ha sthe effect that you cannot have references in the XML to an object of such a type. I see no tautology here, but I am no native speaker.

Change of visibility: I did not say that it breaks binary compatibility, I said it changes the API. While package-private is not that different, we don't ajdust visibility for the sake of tests. If someone accesses the field using relfection, he knows (well, he should) that he cannot rely on binary compatibility anymore.

Thanks for your patience.

@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav May 13, 2015

hey @joehni I have not forgotten about this issue nor have I run out of patience, work has just gotten really dense lately and I've had little time to spare. At the very latest I should be able to get back to this stuff by Sunday.

Sorry for the delay, and thanks for your time!

-Geoff

hey @joehni I have not forgotten about this issue nor have I run out of patience, work has just gotten really dense lately and I've had little time to spare. At the very latest I should be able to get back to this stuff by Sunday.

Sorry for the delay, and thanks for your time!

-Geoff

@Groostav Groostav changed the title from copied work from https://github.com/Groostav/XStream-GG/pull/2/files to Xstream keeps path references to unreferenceable objects May 13, 2015

@joehni

This comment has been minimized.

Show comment
Hide comment
@joehni

joehni May 14, 2015

Member

Hi Goeff, no hurry. We're still busy saving bits and pieces from Codehaus. Officially it shuts down on the weekend.

Member

joehni commented May 14, 2015

Hi Goeff, no hurry. We're still busy saving bits and pieces from Codehaus. Officially it shuts down on the weekend.

@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav May 18, 2015

So, have you considered that the algorithm you're suggesting:

if reference is not null, check if the type is referenceable (immutability does not matter here). If not, throw ConverterException("Unrefernceable type") else continue to get the cached value

The problem with this is the requirement that null values can be referenced. From the tests I'm using it seems null values dont have a type, which means we cant find out if they're referenceable, meaning references to null values are treated like references to an unreferencable type, by that system.

I'm not sure why null values types aren't found. looking at the failing tests, the types alias is the name of the element, its just not beling looked up properly. Any idea why?

For my codebase at least, a JUL log at level warning would suffice (and fail any tests that involve it). Could we do something like that?

So, have you considered that the algorithm you're suggesting:

if reference is not null, check if the type is referenceable (immutability does not matter here). If not, throw ConverterException("Unrefernceable type") else continue to get the cached value

The problem with this is the requirement that null values can be referenced. From the tests I'm using it seems null values dont have a type, which means we cant find out if they're referenceable, meaning references to null values are treated like references to an unreferencable type, by that system.

I'm not sure why null values types aren't found. looking at the failing tests, the types alias is the name of the element, its just not beling looked up properly. Any idea why?

For my codebase at least, a JUL log at level warning would suffice (and fail any tests that involve it). Could we do something like that?

@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav May 18, 2015

also @joehni, before merging I'll squash all this work into one commit, so I wont clutter your history with fixed PR

But I think the ball is back in your court regarding how we want to treat these failing tests.

Let me know

also @joehni, before merging I'll squash all this work into one commit, so I wont clutter your history with fixed PR

But I think the ball is back in your court regarding how we want to treat these failing tests.

Let me know

@joehni

This comment has been minimized.

Show comment
Hide comment
@joehni

joehni May 18, 2015

Member

The build is failing, because of Java 7 syntax. See details of the Travis build above.

Member

joehni commented May 18, 2015

The build is failing, because of Java 7 syntax. See details of the Travis build above.

@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav May 19, 2015

yeah removed that. Now its failing for the above mentioned reasons. Any ideas?

yeah removed that. Now its failing for the above mentioned reasons. Any ideas?

@joehni

This comment has been minimized.

Show comment
Hide comment
@joehni

joehni May 22, 2015

Member

The unit tests of XStream often give interesting surprises ;-)
I'll have a closer look when I am back from holiday. I'll merge the code locally and see what can be done about it. However, I'm offline next 3 weeks ...

Member

joehni commented May 22, 2015

The unit tests of XStream often give interesting surprises ;-)
I'll have a closer look when I am back from holiday. I'll merge the code locally and see what can be done about it. However, I'm offline next 3 weeks ...

Groostav added some commits Jun 10, 2015

Implemented Jira issue-773 (github PR #12)
- currently has problem in the event that the type cannot be deteremined but null values can be referenced -> an immutable type with a null value can be referenced. Doesnt work -> breaks tests.
@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav Jun 10, 2015

ok @joehni:

  • I pushed a change to your suggested algorithm: now the isReferenceable check is more optimistic in that if the type is null then we assume we should keep a reference. I'm not sure how this is going to impact my desired memory behaviour.
  • I also collapsed the work down to 2 much more reasonable commits.

let me know when this makes it in! Im anxious to get my project off my home-built XStream fork.

ok @joehni:

  • I pushed a change to your suggested algorithm: now the isReferenceable check is more optimistic in that if the type is null then we assume we should keep a reference. I'm not sure how this is going to impact my desired memory behaviour.
  • I also collapsed the work down to 2 much more reasonable commits.

let me know when this makes it in! Im anxious to get my project off my home-built XStream fork.

@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav Jun 29, 2015

@joehni are you still there?

@joehni are you still there?

@joehni

This comment has been minimized.

Show comment
Hide comment
@joehni

joehni Jun 29, 2015

Member

Am Montag, 29. Juni 2015, 14:50:18 schrieb Geoff:

@joehni are you still there?


Reply to this email directly or view it on GitHub:
#3 (comment)

Yes, but I am struggling with offline stuff. Since I am not very familiar
neither with Git nor GitHub, I tried to migrate easier stuff first to get a
feeling for the process.

Member

joehni commented Jun 29, 2015

Am Montag, 29. Juni 2015, 14:50:18 schrieb Geoff:

@joehni are you still there?


Reply to this email directly or view it on GitHub:
#3 (comment)

Yes, but I am struggling with offline stuff. Since I am not very familiar
neither with Git nor GitHub, I tried to migrate easier stuff first to get a
feeling for the process.

@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav Jul 3, 2015

fair enough! I've spent my entire (read: 4 year) career on git and I'm still consistently surprised by its behaviour. Give me a shout (by typing @Groostav) if you need me to do anything else. In the meantime I consider the ball to be in your court.

Groostav commented Jul 3, 2015

fair enough! I've spent my entire (read: 4 year) career on git and I'm still consistently surprised by its behaviour. Give me a shout (by typing @Groostav) if you need me to do anything else. In the meantime I consider the ball to be in your court.

@joehni joehni closed this in 5526a2b Jul 13, 2015

@joehni joehni added the enhancement label Jul 13, 2015

@joehni joehni added this to the 1.4.x milestone Jul 13, 2015

joehni added a commit that referenced this pull request Jul 13, 2015

Minimize memory footprint for immutable types at deserialization time,
closes #3. Add compatibility support for already persisted streams.
@Groostav

This comment has been minimized.

Show comment
Hide comment
@Groostav

Groostav Jul 21, 2015

Thanks for this @joehni, sorry for pestering you so much!

Thanks for this @joehni, sorry for pestering you so much!

@joehni

This comment has been minimized.

Show comment
Hide comment
@joehni

joehni Jul 21, 2015

Member

Hey, thanks for your contribution and patience! Keeping a low memory footprint is quite important.

Member

joehni commented Jul 21, 2015

Hey, thanks for your contribution and patience! Keeping a low memory footprint is quite important.

@joehni joehni modified the milestones: 1.4.x, 1.4.9 Mar 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment