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

Extend PolymorphicJsonAdapterFactory to allow custom handling of unknown labels #1094

Merged

Conversation

macisamuele
Copy link
Contributor

@macisamuele macisamuele commented Feb 9, 2020

This PR includes an implementation proposal for #784.

The main idea is to allow users of the JsonAdapterFactory to provide a JsonAdapter that will deal with the reader in case of an unknown/unmapped label.

Doing this has the advantage to give freedom to developers to re-use already existing components and tools to build their logic.

@macisamuele macisamuele force-pushed the maci-extend-PolymorphicJsonAdapterFactory branch from 7242c01 to c81b2ab Compare February 10, 2020 22:00
@macisamuele
Copy link
Contributor Author

Ping @JakeWharton, @ZacSweers as last developers taking care of merging PRs.

As mentioned in the issue I'm working on Yelp/swagger-gradle-codegen#103 and having this merged would simplify a lot the process as well as it would solve #784 by providing flexibility to developers of managing the "edge-case" of unknown labels as they like.

I'd like to have some feedbacks around this PR or at least an ETA 😄 .

@macisamuele macisamuele force-pushed the maci-extend-PolymorphicJsonAdapterFactory branch from c81b2ab to a184e1a Compare February 26, 2020 19:23
@macisamuele macisamuele force-pushed the maci-extend-PolymorphicJsonAdapterFactory branch from a184e1a to 9fad705 Compare February 26, 2020 19:31
macisamuele added a commit to macisamuele/swagger-gradle-codegen that referenced this pull request Feb 26, 2020
@ZacSweers
Copy link
Collaborator

Sorry for the delay. In the future - tagging individual users based on perceived activity isn't the best approach and would appreciate if you don't do that.

I like the idea behind this approach, but have a few thoughts:

  • Should we deprecate withDefaultValue()? Or just keep it as a shorthand for that specific case?
  • I don't think we should expose an entire adapter in the API, as we only want this for reads. This gets to a larger point though, maybe we should have read and write interfaces (this could play well into some cases we want to support in the future)

@macisamuele
Copy link
Contributor Author

@ZacSweers , sorry for the direct ping. I apologize for that.

Should we deprecate withDefaultValue()?

Ideally yes, as already visible on the PR the method does return a JsonAdapter that essentially does very little other than returning the value.
I would be more than happy to annotate the method as deprecated, I was mostly looking for a feedback from the owners before doing it.

I don't think we should expose an entire adapter in the API, as we only want this for reads. This gets to a larger point though, maybe we should have read and write interfaces (this could play well into some cases we want to support in the future)

I understand your point, but consider the following case in Yelp/swagger-gradle-codegen#103

class Animal(open var type: String)
data class Dog(override var type: String, var name: String): Animal(type=type)
data class Cat(override var type: String, var size: Int): Animal(type=type)

If we would allow "custom" handling on unrecognized labels we might have {"type":"unknown"} that will be converted (via the usage of the PolymorphicJsonAdapterFactory) into Animal(type="unknown"). At this point how can we re-serialize the kotlin-object?

The above example is mostly meant to provide a realistic use-case where we don't only need read but also writes.

Said so I'm open to ideas to implement it differently, but I thought that giving the freedom to inject a JsonAdapter as fallback was the easiest to implement, to understand (from a user point-of-view) and more importantly the one that allows the greatest customization.

Before I do update the PR by annotating withDefaultValue as deprecated I'd like to understand if the approach proposed in this PR is a viable approach.

@macisamuele
Copy link
Contributor Author

Are there any updates on this?

macisamuele added a commit to macisamuele/swagger-gradle-codegen that referenced this pull request Jun 12, 2020
@swankjesse
Copy link
Member

@macisamuele I like the idea, lets go forward with this.

@swankjesse swankjesse merged commit cd31e5c into square:master Jul 31, 2020
@swankjesse
Copy link
Member

Small follow-up: #1182

@swankjesse
Copy link
Member

Nice PR by the way. I apologize for being slow-to-respond. The last several months have been... hectic.

@macisamuele macisamuele deleted the maci-extend-PolymorphicJsonAdapterFactory branch August 2, 2020 20:53
macisamuele added a commit to macisamuele/swagger-gradle-codegen that referenced this pull request Oct 24, 2020
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.

None yet

3 participants