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

Docs falsely state TaggedFieldSerializer doesn't read deprecated fields #522

Closed
cypherdare opened this issue Jun 12, 2017 · 2 comments
Closed

Comments

@cypherdare
Copy link
Contributor

The docs say "fields marked with the @Deprecated annotation will be ignored when reading old bytes", but this is not the case. The read() method reads all tagged fields that it encounters, regardless of whether they are deprecated.

It makes sense that it does this. Deprecated fields still have to be read if encountered, so the stream will be at the correct location for subsequent fields and objects in the stream.

Some alternative ways to fix this:

  1. Fix the docs to say that deprecated fields are not written, but they are still read if encountered.

  2. Change functionality so objects within the deprecated field are read but not assigned to the field. Users must take care that those objects cannot safely be lost to the GC. For example, if they require native memory cleanup. Anyone in this situation has already been leaking memory if they were relying on the feature as described in the docs.

Going off of option 2:
#521 adds the annexed option to Tags so they are encoded chunked. Maybe rename annexed to skippable, and it serves a dual purpose. Skippable fields have the extra bonus feature of not being read if they get deprecated. I'm not sure if this adds enough value to be worth the more complicated API, though. You would need to anticipate which fields might become deprecated later, and eat the cost of chunked encoding for the possible benefit of slightly faster read times (but only when old data is read in newer applications).

I personally think option 1 makes more sense. It allows manual cleanup of deprecated fields in case that's necessary, and the user could easily null-out the deprecated fields. Or we could add an option to TaggedFieldSerializerConfig to turn on option 2.

@cypherdare cypherdare changed the title Docs falsely claim TaggedFieldSerializer doesn't read deprecated fields Docs falsely state TaggedFieldSerializer doesn't read deprecated fields Jun 12, 2017
@magro
Copy link
Collaborator

magro commented Jul 13, 2017

Option 1 sounds good btw! :-)

@NathanSweet
Copy link
Member

I've fixed this in a local branch, this issue will be closed when it is merged.

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

No branches or pull requests

3 participants