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

Improve versioning support documentation and validate version #2214

Conversation

Marcono1234
Copy link
Collaborator

Resolves #1007

Resolves that issue by considering it a documentation issue, not an implementation issue. When @Until was added by 6f59bc3, a corresponding test was also added which checks that the @Until version is exclusive. From the exclusion standpoint this behavior makes sense since you specify that a field exists until version X in which it is removed. This also allows combinations of @Until(X) marking the old fields and @Since(X) marking the new fields in a class without overlap. If the value was inclusive it would create situations where for version X a field is included, but for version X.00001 it is not included anymore.
Though on the other hand maybe there are situations where it is desired that the value is inclusive, see StackOverflow question on which #1007 is based.

@Marcono1234 Marcono1234 force-pushed the marcono1234/versioning-documentation branch from 21c2c82 to ea859e9 Compare October 2, 2022 18:12
public GsonBuilder setVersion(double ignoreVersionsAfter) {
excluder = excluder.withVersion(ignoreVersionsAfter);
public GsonBuilder setVersion(double version) {
if (Double.isNaN(version) || version < 0.0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have added this < 0.0 check because negative version numbers are probably rather uncommon and this could cause collisions with the undocumented value -1.0 representing no version:

private static final double IGNORE_VERSIONS = -1.0d;

if (annotationVersion > version) {
return false;
}
return version >= annotationVersion;
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Oct 2, 2022

Choose a reason for hiding this comment

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

This should behave equivalently (unless someone uses NaN as version ...), but is hopefully a bit easier to understand.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, the old code was surprising.

if (annotationVersion <= version) {
return false;
}
return version < annotationVersion;
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Oct 2, 2022

Choose a reason for hiding this comment

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

This should behave equivalently (unless someone uses NaN as version ...), but is hopefully a bit easier to understand.

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

This seems reasonable. It's technically an incompatible change, since we never said version numbers could not be negative. But I think it would be pretty surprising if someone were using this with negative version numbers. (For what it's worth, there appears to be exactly one user of GsonBuilder.setVersion in Google's source code, outside Gson's own tests.)

@eamonnmcmanus eamonnmcmanus merged commit 796193d into google:master Oct 2, 2022
@Marcono1234 Marcono1234 deleted the marcono1234/versioning-documentation branch October 3, 2022 11:27
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.

Excluder rejects field when version exceeds or is equal to version in @Until
2 participants