Support unrecognized properties for EV features by dzinad · Pull Request #1500 · mapbox/mapbox-java

dzinad · 2022-10-19T09:13:49Z

No description provided.

sonatype-lift · 2022-10-19T09:15:50Z

⚠️ 5 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

LukasPaczos · 2022-10-19T09:44:43Z

+     * @param unrecognizedProperties parameters to add to request
+     */
+    @NonNull
+    public Builder unrecognizedProperties(


Why not make the abstract unrecognized method public in DirectionsJsonObject.Builder? We definitely see a use-case for this method being public. Or add unrecognizedProperties as public function to the abstract builder if we don't want to expose the SerializableJsonElement.

cc @VysotskiVadim

It wasn't public so I thought there was a reason for it. So we should only make it public when needed, a white list, not black list;

The unrecognized() method accepts Map<String, SerializableJsonElement> which is not very convenient for the end user, so yes, I'd prefer adding unrecognizedProperties with Map<String, JsonElement> instead.

So is there a reason why it's not public?

LukasPaczos · 2022-10-19T09:45:44Z

+/**
+ * Provides utility methods to work with unrecognized properties.
+ */
+public final class UnrecognizedPropertiesUtils {


Should this be public? DirectionsJsonObject already exposes these accessors.

I want to access it from the service-directions-refresh-models module. To avoid code duplicating.

I imagine we don't need this if we expose the map getters.

LukasPaczos · 2022-10-19T09:46:18Z

+
+  abstract static class Builder<T extends Builder> {
+    @NonNull
+    abstract T unrecognized(@Nullable Map<String, SerializableJsonElement> value);


Same as above, why not making this public?

LukasPaczos · 2022-10-19T09:48:20Z

+
+  @Nullable
+  @UnrecognizedJsonProperties
+  abstract Map<String, SerializableJsonElement> unrecognized();


We could consider making this public, refs NAVAND-778. Doesn't have to happen in this PR though.

How will making this public help NAVAND-778? For the case described there it's more like adding a method addUnrecognizedProperties instead of the current one that resets them.

A public getter would allow us to first get the properties map, mutate them, and set them back. As we would do with any other builder object parameter. The getter and setter methods in this case need to work on the same map type so that developers don't need to do any parsing themselves, that's why I was hoping to expose a single function in the abstract class instead of adding explicit getter and setters manually everywhere.

OK, makes sense but it's not very convenient to use with SerializableJsonElement. But I guess we could add convenience methods directly to DirectionsJsonObject and DirectionsRefreshJsonObject. WDYT?

Sure, as we're discussing in #1500 (comment) JsonElement might be better.

LukasPaczos · 2022-10-19T09:50:37Z

+     */
+    @NonNull
+    public Builder unrecognizedProperties(
+      @Nullable Map<String, JsonElement> unrecognizedProperties


RouteOptions.Builder exposes this map as Map<String, String>, we should align this one way or the other. For the general purpose JsonElement does look like a better candidate, but again only if we don't want to expose the SerializableJsonElement. @VysotskiVadim I remember you had some thoughts about it in the past.

Yes, here we definitely need JsonElement. The structure will be complex.
For RouteOptions it's only about get parameters, that's why strings are used.
These are different use cases so I'm not sure if we should complicate RouteOptions usage with converting it all to JsonElement. But we could have 2 methods for RouteOptions to keep both consistency and usability.

@VysotskiVadim I remember you had some thoughts about it in the past.

the initial idea was to expose as little as possible to simplify future changes.

For RouteOptions it's only about get parameters, that's why strings are used.

that's right.

These are different use cases so I'm not sure if we should complicate RouteOptions usage with converting it all to JsonElement.

I agree with @dzinad. I don't see value in aligning unrecognised properties API between request and response models.

I don't fully agree. Whether we look at request or response object, they are all DirectionsJsonObjects (or refresh) and support the unrecognized fields in exactly the same way, and that's why I think they should also expose the same API to mutate or reset the properties.

We started with RouteOptions needing some form of unrecognized properties setter, now we need to add the same to LegAnnotation and this could be true at any point in time for any DirectionsJsonObject, depending on what's experimental in the Directions API at a certain time.

Couldn't we expose Map<String, JsonElement> getter and setters for all DirectionsJsonObjects?

We can if we leave the convenience method for RouteOptions.

Why couldn't we use only Map<String, JsonElement> for RouteOptions as well?

The only change developers would have to make is wrap their string values in JsonPrimitive(value). That's actually even more flexible because other than string parameters are also acceptable by the Directions API. Of course we can leave the existing function in place but I'd rather deprecate it.

It's a breaking change and that's something that would definitely affect the users;

In the majority of the cases users only need Map<String, String>. Making them use Map<String, JsonElement> will force them to write more boiler-plate code on their side.

I'm strongly in favour of leaving the convenience method as-is. We'll provide a more flexible options: if they want something other than strings, they can use the new method. And I'm not suggesting to add Map<String, String> everywhere: only leave it where it already is and where it makes sense to have it.

It's a breaking change and that's something that would definitely affect the users;

We'd not be removing the old function, only deprecating it.

In the majority of the cases users only need Map<String, String>. Making them use Map<String, JsonElement> will force them to write more boiler-plate code on their side.

Map<String, String> is limiting and actually somewhat incorrect. For example, look at ev_max_charge parameter in https://docs.mapbox.com/api/navigation/directions/#parameters-for-electric-vehicle-routing - it's an integer but we'd recommend (and today require) developers pass it as a string. JsonPrimitive(integer) would be more accurate here.

In any case, we can discuss and consider the deprecation separately, the old function has to stay in place regardless.

vadzim-vys · 2022-10-19T10:03:41Z

 * Provides a base class for Directions model classes.
 */
-public class DirectionsRefreshJsonObject implements Serializable {
+public abstract class DirectionsRefreshJsonObject implements Serializable {


it's a breaking change, isn't it? Users who create the object won't be able to this, right? TBH I don't remember which visibility default constructor from java has 🙂

even if it's a breaking change we can consider the previous implementation as a bug. I believe we already did it once 🙂

TBH I don't remember which visibility default constructor from java has

It's public with no arguments.

it's a breaking change, isn't it?

You are right. Good catch! I see the following options here:

Commit the breaking change and reduce the constructor visibility for the future. Why would anyone create an empty DirectionsRefreshJsonObject?.. But that would still break semver;

Up the major version. I'm not sure about the policy on that. @LukasPaczos could you chime in here?

Add unrecognized properties directly to DirectionsRouteRefresh. That's the only place they are needed right now. But it's not very flexible;

Add compile-time support for EV features. Not really consistent with the feature being in beta;

Actually I had the same problem adding unrecognized properties: https://github.com/mapbox/mapbox-java/pull/1394/files#r848311760 😄

Haha, nice
Then maybe we should think about reducing the visibility of everything that should not be used directly? These are mostly constructors but it makes sense to double-check. That's a separate issue though.

Let's make the change as last time around 👍

Then maybe we should think about reducing the visibility of everything that should not be used directly?

Good ticket to have for the next major version 👍

NAVAND-802

LukasPaczos

LGTM

dzinad requested a review from a team as a code owner October 19, 2022 09:13

dzinad force-pushed the NAVAND-713-dd branch from cdfe080 to 6278a0c Compare October 19, 2022 09:15

dzinad force-pushed the NAVAND-713-dd branch from 6278a0c to 6c075ac Compare October 19, 2022 09:37

LukasPaczos reviewed Oct 19, 2022

View reviewed changes

vadzim-vys reviewed Oct 19, 2022

View reviewed changes

Comment thread ...ls/src/main/java/com/mapbox/api/directionsrefresh/v1/models/DirectionsRefreshJsonObject.java

vadzim-vys reviewed Oct 19, 2022

View reviewed changes

Comment thread services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/LegAnnotation.java Outdated

dzinad force-pushed the NAVAND-713-dd branch 3 times, most recently from a3e8487 to 7be690f Compare October 20, 2022 16:00

LukasPaczos approved these changes Oct 20, 2022

View reviewed changes

dzinad enabled auto-merge (rebase) October 20, 2022 18:22

dzinad force-pushed the NAVAND-713-dd branch from 7be690f to f19100a Compare October 20, 2022 18:38

NAVAND-713: publicly expose unrecognized properties

ce8b93b

dzinad force-pushed the NAVAND-713-dd branch from f19100a to ce8b93b Compare October 20, 2022 18:50

dzinad merged commit 0b0164f into main Oct 20, 2022

dzinad deleted the NAVAND-713-dd branch October 20, 2022 19:03

LukasPaczos mentioned this pull request Oct 21, 2022

updated mapbox-java to v6.9.0-beta.1 mapbox/mapbox-navigation-android#6499

Merged

Conversation

dzinad commented Oct 19, 2022

Uh oh!

sonatype-lift Bot commented Oct 19, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LukasPaczos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants