-
Notifications
You must be signed in to change notification settings - Fork 117
Roundtrip of unrecognised JSON properties #1394
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
Changes from all commits
f447f0f
fecee7b
fc59c2a
047da92
f2c7e28
132b796
fd2228b
a8d931f
20b35c2
997c5fd
e1b863c
f6be7b9
b326916
987ba05
2f0c24e
edb8750
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,23 @@ | ||
| package com.mapbox.api.directions.v5.models; | ||
|
|
||
| import androidx.annotation.NonNull; | ||
| import androidx.annotation.Nullable; | ||
| import com.google.gson.GsonBuilder; | ||
| import com.mapbox.api.directions.v5.DirectionsAdapterFactory; | ||
| import com.mapbox.geojson.Point; | ||
| import com.mapbox.geojson.PointAsCoordinatesTypeAdapter; | ||
| import com.mapbox.auto.value.gson.SerializableJsonElement; | ||
| import com.mapbox.auto.value.gson.UnrecognizedJsonProperties; | ||
|
|
||
| import java.io.Serializable; | ||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Provides a base class for Directions model classes. | ||
| * | ||
| * @since 3.4.0 | ||
| */ | ||
| public class DirectionsJsonObject implements Serializable { | ||
| public abstract class DirectionsJsonObject implements Serializable { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a breaking change (not sure how impactful it would actually be in practice). Could we make the methods return empty and implement downstream, how would it look like?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice catch 👍
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AutoValue won't generate fields for not abstract methods
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see a few possible solutions right now. Do breaking changeMaybe we can make ourself believe that it's not really a breaking change using following arguments: it more like a fix of a bug, nobody should have created an instance of WorkaroundI can create an abstract class
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that we need to decide if somebody could be affected by not being able to instantiate
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel confident. I can't imaging a case when somebody instantiate
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @VysotskiVadim I agree. There's no practical reason to extend the |
||
|
|
||
| /** | ||
| * This takes the currently defined values found inside this instance and converts it to a json | ||
|
|
@@ -27,4 +32,13 @@ public String toJson() { | |
| gson.registerTypeAdapter(Point.class, new PointAsCoordinatesTypeAdapter()); | ||
| return gson.create().toJson(this); | ||
| } | ||
|
|
||
| @Nullable | ||
| @UnrecognizedJsonProperties | ||
| abstract Map<String, SerializableJsonElement> unrecognized(); | ||
|
|
||
| abstract static class Builder<T extends Builder> { | ||
| @NonNull | ||
| abstract T unrecognized(Map<String, SerializableJsonElement> value); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| public abstract class RouteOptions extends DirectionsJsonObject { | ||
|
|
||
| private static final String UTF_8 = "UTF-8"; | ||
| private static final String ACCESS_TOKEN_URL_PARAM_NAME = "access_token"; | ||
|
|
||
| /** | ||
| * Build a new instance of {@link RouteOptions} and sets default values for: | ||
|
|
@@ -889,6 +890,9 @@ public static RouteOptions fromUrl(@NonNull URL url) { | |
| int idx = query.indexOf("="); | ||
| String property = URLDecoder.decode(query.substring(0, idx), UTF_8); | ||
| String value = URLDecoder.decode(query.substring(idx + 1), UTF_8); | ||
| if (property.equals(ACCESS_TOKEN_URL_PARAM_NAME)) { | ||
| continue; | ||
| } | ||
|
Comment on lines
+893
to
+895
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a test that covers this already?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, I added this line because one of tests failed |
||
|
|
||
| boolean isExperimental = false; | ||
| for (int i = 0; i < Experimental.experimentalParameters.length; i++) { | ||
|
|
@@ -937,7 +941,7 @@ public URL toUrl(@NonNull String accessToken) { | |
| .append(String.format("/%s", user())) | ||
| .append(String.format("/%s", profile())) | ||
| .append(String.format("/%s", coordinates())) | ||
| .append(String.format("?access_token=%s", accessToken)) | ||
| .append(String.format("?%s=%s", ACCESS_TOKEN_URL_PARAM_NAME, accessToken)) | ||
| .append(String.format("&geometries=%s", geometries())); | ||
|
|
||
| if (alternatives() != null) { | ||
|
|
@@ -1134,7 +1138,7 @@ private static RouteOptions fromJsonString(@NonNull String json) { | |
| * This builder can be used to set the values describing the {@link RouteOptions}. | ||
| */ | ||
| @AutoValue.Builder | ||
| public abstract static class Builder { | ||
| public abstract static class Builder extends DirectionsJsonObject.Builder<Builder> { | ||
|
|
||
| /** | ||
| * Base URL for the request. | ||
|
|
@@ -2191,7 +2195,7 @@ public static Experimental fromJson(String json) { | |
| * This builder can be used to set the values describing the {@link Experimental}. | ||
| */ | ||
| @AutoValue.Builder | ||
| public abstract static class Builder { | ||
| public abstract static class Builder extends DirectionsJsonObject.Builder<Builder> { | ||
|
|
||
| /** | ||
| * Object representing experimental value. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.