feat: [TSI-2083] enable format_options argument for java-client #426

hahmed-dev · 2023-10-12T11:23:32Z

Assumptions

Some assumptions made while working on the PR:

format_options argument will be key-value based map with values mostly being of primitive types
Initially explored the possibility of using params with style deepObject and explode: true option as suggested here as a fix but subsquently wasn't able to find appropriate mustache directive to determine if the param has any of these attributes (deepObject, explode: true) which led to adding a fix by checking if input is of the type Map and if yes, manually convert it to key value based Pair objects.

Problem Statement

Phrase API accepts format_options argument on locales download endpoint.

For java-client the format_option argument was not being properly parsed and embedded in the query params resulting in a request url like below

https://api.phrase.com/v2/projects/e4c0450ccc8dcc087fac8351ecc89eb6/locales/01877decc84124a75fdd0a4fc147d8de/download?file_format=properties&tags=menu&format_options=%7B%22omit_separator_space%22%3Atrue%7D&encoding=UTF-8

Solution

This pull requests adds the ability to parse any query params of type map and embed them to the URL's query string as proper key value pairs resulting in request URLs like:

localhost:3000/api/v2/projects/:project_id/locales/:id/download?file_format=i18next_4&format_options%5Bnesting%5D=false

doc/compiled.json

paths/locales/download.yaml

jablan · 2023-10-13T09:50:13Z

openapi-generator/templates/java/libraries/okhttp-gson/api.mustache

@@ -100,7 +100,12 @@ public class {{classname}} {
        {{javaUtilPrefix}}List<Pair> localVarCollectionQueryParams = new {{javaUtilPrefix}}ArrayList<Pair>();
        {{#queryParams}}
        if ({{paramName}} != null) {
+            {{#isMap}}


I believe you should use #isDeepObject instead. You can see it's available to the API template if you run generate with --global-property debugOperations=true (also isExplode is available)

Thanks, will take a look, the document it printed out for me upon running with this argument,

niether included any documentations for get end points

nor isDeepObject or isExplode properties

But let me check again, would be more specific

or, you can pass {{style}} as a parameter to mappedParameterToPairs, and then handle it appropriately (leaving other styles not covered until we need them).

note that there are different debug... flags possible to pass as global-property. https://openapi-generator.tech/docs/debugging/#templates

Will check this. Thanks for sharing.

Thanks Mladen, using debugOperations was able to find the name of the directive.

jablan · 2023-10-13T09:52:36Z

openapi-generator/templates/java/libraries/okhttp-gson/ApiClient.mustache

+    public List<Pair> mappedParameterToPairs(String name, Object parameter){
+        List<Pair> params = new ArrayList<Pair>();
+
+        if(parameter instanceof Map){


there's additional problem that we'll have to address at some point: these objects can be nested. so, it should be possible to generate a parameter such as formatOptions[customMetadataColumns][order]=B and so on 😄 Perhaps we could address that in a separate ticket

Thats true, i'll check if its something that can be addressed already within this PR otherwise we can do it separately :)

..and arrays :D

I guess in case if a value against format_option is an array, it should already be handled? If not I will check and see if that can be addressed as well. I think from just a look maybe keynames need to be constructed in a nested way.

seems it covers the basic case, but I guess arrays can also be nested 😬 never mind, we can cross that bridge when we get there

Also go will probably have the same issue with customMetadataColumns https://github.com/phrase/phrase-go/blob/master/api_locales.go#L436

Was already looking into nested arguments, that part is addressed with the latest commit. Regarding values being arrays/nested array, can see if it can be handled separately.

@theSoenke by same issue do you mean, it will require same patch like this?? For map based query params?

@hahmed-dev format_options should already work. But for more deeply nested params it will need a similar fix I think

@theSoenke I think deep nesting is an open feature request in the open api spec. If thats the case we may have to find a work around whenever we encounter something like this.

jablan

nice!

jablan · 2023-10-13T13:32:12Z

clients/java/src/test/java/com/phrase/client/api/LocalesApiTest.java

@@ -168,7 +173,8 @@ public void localeDownloadTest() throws ApiException, IOException, InterruptedEx
        Assert.assertEquals("Correct file contents", fileContents, body);

        RecordedRequest recordedRequest = mockBackend.takeRequest();
-        Assert.assertEquals("Request path", "//projects/MY_PROJECT_ID/locales/MY_ID/download?format_options%5Bomit_separator_space%5D=true&format_options%5Bfallback_language%5D=en", recordedRequest.getPath());
+        // for some reason with deep nested query params, ordering of query params change


rails implementation simply sorts the params by name at end. :)

docstun · 2023-10-30T07:03:11Z

@hahmed-dev is this still valid?

hahmed-dev · 2023-10-30T07:58:01Z

@docstun yes, we initially were waiting to merge the open api version bump PR since its based out of that, but we discussed last week, there is no dependency and we can merge this independently as well. I will try and shortly open a new PR and if that gets merged close this one.

hahmed-dev changed the title ~~Tsi 2083 java format~~ feat: [TSI-2083] enable format_options argument for java-client Oct 12, 2023

hahmed-dev marked this pull request as ready for review October 12, 2023 16:12

hahmed-dev requested review from jablan and theSoenke October 12, 2023 16:12

hahmed-dev commented Oct 12, 2023

View reviewed changes

doc/compiled.json Show resolved Hide resolved

hahmed-dev commented Oct 13, 2023

View reviewed changes

paths/locales/download.yaml Show resolved Hide resolved

jablan reviewed Oct 13, 2023

View reviewed changes

jablan approved these changes Oct 13, 2023

View reviewed changes

Base automatically changed from tsi-1874-update-generator-v7 to master October 30, 2023 14:55

hahmed-dev and others added 11 commits October 30, 2023 17:52

TSI-2083 update java client generation file

453eb65

tsi-2083 update test file

244d233

testing out changes with different string formation

df4e052

Add typecasting workaround to test build

1a61838

Updated assertion to match encoded params

718bd6d

try improving gradle test output

85ae2fc

try improving gradle test output

9078009

try improving gradle test output

b70c0df

Remove redundant line

cf52973

Adds ability to parse and embed nested params

29770d2

Add deep object directive

5f66775

hahmed-dev force-pushed the tsi-2083-java-format branch from d11a02b to 5f66775 Compare October 30, 2023 16:52

jablan approved these changes Oct 30, 2023

View reviewed changes

hahmed-dev merged commit faa8cb3 into master Oct 30, 2023
10 checks passed

hahmed-dev deleted the tsi-2083-java-format branch October 30, 2023 17:01

This was referenced Oct 30, 2023

chore(master): release php 1.17.0 #456

Merged

chore(master): release go 2.18.0 #457

Merged

chore(master): release python 1.17.0 #458

Merged

This was referenced Oct 30, 2023

chore(master): release ruby 2.20.0 #459

Merged

chore(master): release cli 2.15.0 #451

Merged

chore(master): release java 1.17.0 #460

Merged

chore(master): release typescript 1.17.0 #461

Merged

phrase-bot mentioned this pull request Apr 10, 2024

chore(master): release php 1.22.0 #561

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [TSI-2083] enable format_options argument for java-client #426

feat: [TSI-2083] enable format_options argument for java-client #426

hahmed-dev commented Oct 12, 2023 •

edited

Loading

jablan Oct 13, 2023

hahmed-dev Oct 13, 2023

jablan Oct 13, 2023

jablan Oct 13, 2023

hahmed-dev Oct 13, 2023 •

edited

Loading

hahmed-dev Oct 13, 2023

jablan Oct 13, 2023

hahmed-dev Oct 13, 2023

jablan Oct 13, 2023

hahmed-dev Oct 13, 2023 •

edited

Loading

jablan Oct 13, 2023

theSoenke Oct 13, 2023

hahmed-dev Oct 13, 2023

hahmed-dev Oct 13, 2023

theSoenke Oct 13, 2023

hahmed-dev Oct 13, 2023

jablan left a comment

jablan Oct 13, 2023

docstun commented Oct 30, 2023

hahmed-dev commented Oct 30, 2023

feat: [TSI-2083] enable format_options argument for java-client #426

feat: [TSI-2083] enable format_options argument for java-client #426

Conversation

hahmed-dev commented Oct 12, 2023 • edited Loading

Assumptions

Problem Statement

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahmed-dev Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahmed-dev Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jablan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

docstun commented Oct 30, 2023

hahmed-dev commented Oct 30, 2023

hahmed-dev commented Oct 12, 2023 •

edited

Loading

hahmed-dev Oct 13, 2023 •

edited

Loading

hahmed-dev Oct 13, 2023 •

edited

Loading