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

Kotlin client: add volley library support #10253

Merged
merged 114 commits into from
Dec 20, 2021
Merged

Conversation

alisters
Copy link
Contributor

@alisters alisters commented Aug 25, 2021

Add a volley library to the kotllin client

#10011

I will add design notes to the above issue, but will move those notes to appropriate locations in the documentation when i know where they should reside.

@jimschubert @wing328

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

alister995199 and others added 30 commits July 12, 2021 14:14
…, add extra repository for Android Gradle build tools
@alisters
Copy link
Contributor Author

@4brunu - i still need to regenerate the samples. That might not happen till tomorrow (Australia time)

@4brunu
Copy link
Contributor

4brunu commented Dec 13, 2021

Sure, take your time 👍

@alisters
Copy link
Contributor Author

@4brunu - samples partially regenerated. See comment above - #10253 (comment).

import java.time.OffsetDateTime
import java.util.UUID

object Serializer {
Copy link
Contributor

Choose a reason for hiding this comment

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

@alisters I just noticed that this class is not used, which means that, OffsetDateTimeAdapter, LocalDateAdapter, ByteArrayAdapter are also not used.
Could you please remove the reference to them in the KotlinClientCodegen.java?
You should remove them from the supportingFiles

Copy link
Contributor Author

@alisters alisters Dec 16, 2021

Choose a reason for hiding this comment

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

The adapter classes are used inside GsonRequest - src/main/resources/kotlin-client/libraries/jvm-volley/request/GsonRequest.mustache

I've removed the Serialzer as a supporting file. In processJVMVolleyLibrary
addSupportingSerializerAdapters(infrastructureFolder);
supportingFiles.remove(new SupportingFile("jvm-common/infrastructure/Serializer.kt.mustache", infrastructureFolder, "Serializer.kt"));
See d05a42b

The serializer class is part of jvm-common - src/main/resources/kotlin-client/jvm-common/infrastructure/Serializer.kt.mustache. So we left it in the supporting files that are generated when gson is used as the serializer, even though our library uses gson, but doesn't use it. I guess we avoided another conditional in kotlin code gen for volley AND gson - it's added addSupportingSerializerAdapters, which only knows about serializers at the moment.

Let me know if this fixup is ok. Happy to fix some other way if there is a cleaner way to do it.

Comment on lines +706 to +709
supportingFiles.add(new SupportingFile("build.mustache", "", "build.gradle"));
supportingFiles.add(new SupportingFile("gradle.properties.mustache", "", "gradle.properties"));
supportingFiles.add(new SupportingFile("settings.gradle.mustache", "", "settings.gradle"));
supportingFiles.add(new SupportingFile("manifest.mustache", "", "src/main/AndroidManifest.xml"));
Copy link
Contributor

Choose a reason for hiding this comment

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

@alisters just wondering, how different are those grade files from the other shared gradle files?
If they are very different, we should keep them separated, if they are similar, we should consider merge them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The build.mustache/build.gradle is very different on account of android versus straight jvm/java builds.
Same goes for the manifest - Android only.
Other builds don't have a gradle properties - i could fold that into the build.gradle i think. Pretty new to gradle so i think myself or colleague Steve just made one by default - no strong rationale there.
The settings.gradle is the shared one - just keeping the conditional logic a bit cleaner there i guess ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me 👍

@4brunu
Copy link
Contributor

4brunu commented Dec 16, 2021

@alisters I have been investigating the question of have having one gson instance that is shared by all requests vs one gson instance per request, and I think we should go with the shared one, and use the Serializer.kt, since it's thread safe and doesn't keep any state internally.

https://github.com/google/gson/blob/master/UserGuide.md#using-gson

This way we could make the volley generator agnostic from the serializer, and support multiple ones.

So here is my sugestions:
Rename GsonRequest to something else, like Request.
In the GsonRequest.build() stop receiving the type parameter, and receive the response type as generic
Serializer.gson.fromJson(bodyContent, (object: TypeToken<T>(){}).getType())
Serializer.gson.toJson(content, T::class.java)

Basically the usages of gson should be only inside GsonRequest.

With this it will be very easy to all support for all the serializers, moshi, kotlinx serialization, etc.

mediaType==null || (mediaType.startsWith("application/") && mediaType.endsWith("json")) ->
{{#moshi}}Serializer.moshi.adapter<T>().fromJson(bodyContent){{/moshi}}{{#gson}}Serializer.gson.fromJson(bodyContent, (object: TypeToken<T>(){}).getType()){{/gson}}{{#jackson}}Serializer.jacksonObjectMapper.readValue(bodyContent, object: TypeReference<T>() {}){{/jackson}}{{#kotlinx_serialization}}Serializer.jvmJson.decodeFromString<T>(bodyContent){{/kotlinx_serialization}}
else -> throw UnsupportedOperationException("responseBody currently only supports JSON body.")

RequestBody.create(
{{#moshi}}
MediaType.parse(mediaType), Serializer.moshi.adapter(T::class.java).toJson(content)
{{/moshi}}
{{#gson}}
MediaType.parse(mediaType), Serializer.gson.toJson(content, T::class.java)
{{/gson}}
{{#jackson}}
MediaType.parse(mediaType), Serializer.jacksonObjectMapper.writeValueAsString(content)
{{/jackson}}
{{#kotlinx_serialization}}
MediaType.parse(mediaType), Serializer.jvmJson.encodeToString(content)
{{/kotlinx_serialization}}
)

Comment on lines +23 to +24
fun formatDateTime(datetime: OffsetDateTime) = DateTimeFormatter.ISO_INSTANT.format(datetime)
fun formatDate(date: LocalDate) = DateTimeFormatter.ISO_LOCAL_DATE.format(date)
Copy link
Contributor

Choose a reason for hiding this comment

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

@alisters here instead of using the DateFormatters directly, we should use the serializer to keep everything coherent

protected inline fun <reified T: Any> parseDateToQueryString(value : T): String {
/*
.replace("\"", "") converts the json object string to an actual string for the query parameter.
The moshi or gson adapter allows a more generic solution instead of trying to use a native
formatter. It also easily allows to provide a simple way to define a custom date format pattern
inside a gson/moshi adapter.
*/
return Serializer.gson.toJson(value, T::class.java).replace("\"", "")
}

protected inline fun <reified T: Any> parseDateToQueryString(value : T): String {
{{#toJson}}
/*
.replace("\"", "") converts the json object string to an actual string for the query parameter.
The moshi or gson adapter allows a more generic solution instead of trying to use a native
formatter. It also easily allows to provide a simple way to define a custom date format pattern
inside a gson/moshi adapter.
*/
{{#moshi}}
return Serializer.moshi.adapter(T::class.java).toJson(value).replace("\"", "")
{{/moshi}}
{{#gson}}
return Serializer.gson.toJson(value, T::class.java).replace("\"", "")
{{/gson}}
{{#jackson}}
return Serializer.jacksonObjectMapper.writeValueAsString(value).replace("\"", "")
{{/jackson}}
{{#kotlinx_serialization}}
return Serializer.jvmJson.encodeToString(value).replace("\"", "")
{{/kotlinx_serialization}}
{{/toJson}}
{{^toJson}}
return value.toString()
{{/toJson}}
}

Copy link
Contributor Author

@alisters alisters Dec 17, 2021

Choose a reason for hiding this comment

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

@4brunu - i have to think through this one. Would this still allow the application of request factory specific adapters if they are added to a single factory instance ? i agree allowing the multiple serializers is a good move if it can happen.

The other change knock on change for the serializers is the build changes, and being able to test it. Because i'm not a gradle or android guru i don't know off hand how to do that, or what issues i might run into - i'm thinking of transient dependencies of the other serialisation libraries. I can update our library to build, but i'm less sure about what if any other changes would be required to test it in our app with different serializers.

I don't have a test harness setup for testing the pet store example. Does such a thing exist for other libraries/generators ?

I'm on leave for 3 weeks from this afternoon - this work is part work time, part personal time - so i'm not sure when i'll get this done. Are you ok with this potentially restarting in the new year ? I imagine you don't mind if this goes in this release or the next point release ? I guess another way of putting this is do you want to support multiple serializers in this PR, or open a seperate one. I'm happy either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the Serializer you can do Serializer.gsonBuilder.add(...)

What do you mean by test harness setup?

Sure, when you have time 👍
Let's merge this when it's more solid, because now we can change whatever we want, once it's merge, we need to worry about backwards compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

@alisters since this PR was already merge, I would appreciate it if you could continue the changes we discuss here in another PR when you have time 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@4brunu - by test harness i just meant someway to run the generated code with the alternate serializers - thats all. I'll raise a new PR for this serializer related work.

@wing328 wing328 merged commit 0de482d into OpenAPITools:master Dec 20, 2021
@wing328
Copy link
Member

wing328 commented Dec 20, 2021

As discussed with @4brunu , this PR is ready to be included in v5.3.1 so I've merged it.

For enhancements, please file separate PRs.

@wing328
Copy link
Member

wing328 commented Dec 20, 2021

UPDATE: Included the tests in the CI via #11156

@alisters
Copy link
Contributor Author

alisters commented Dec 26, 2021

@wing328 and @4brunu - thank you for merging this and @4brunu - thank you for all the refinements. I'll raise another PR for supporting additional serializers.

10 years and 3 companies ago i tried a contract/api first approach to service design and it was terrible. We use 5 different open api generator libraries now, and it has made the experience so much better. I'm convinced api first design will only get more popular with async api, things like kong and improved service catalogs etc - so thank you for all the hard work you guys put in making this whole ecosystem viable !

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

Successfully merging this pull request may close these issues.

6 participants