feat: [TSI-2083] enable format_options argument for java-client by hahmed-dev · Pull Request #426 · phrase/strings-openapi

Hassan Ahmed (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

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

        {{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

+    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.

Sönke Behrendt (@theSoenke) by same issue do you mean, it will require same patch like this?? For map based query params?

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

Sönke Behrendt (@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


        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. :)

Manuel Boy (docstun) · 2023-10-30T07:03:11Z

Hassan Ahmed (@hahmed-dev) is this still valid?

Hassan Ahmed (hahmed-dev) · 2023-10-30T07:58:01Z

Manuel Boy (@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.

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

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

Hassan Ahmed (hahmed-dev) requested review from jablan and Sönke Behrendt (theSoenke) October 12, 2023 16:12

Hassan Ahmed (hahmed-dev) commented Oct 12, 2023

View reviewed changes

Comment thread doc/compiled.json

Hassan Ahmed (hahmed-dev) commented Oct 13, 2023

View reviewed changes

Comment thread paths/locales/download.yaml

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

Hassan Ahmed (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

Hassan Ahmed (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

Hassan Ahmed (hahmed-dev) merged commit faa8cb3 into master Oct 30, 2023

Hassan Ahmed (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 (phrase-bot) mentioned this pull request Apr 10, 2024

chore(master): release php 1.22.0 #561

Merged

Conversation

Hassan Ahmed (hahmed-dev) commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Assumptions

Problem Statement

Solution

Uh oh!

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!

Hassan Ahmed (hahmed-dev) Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Hassan Ahmed (hahmed-dev) Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jablan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manuel Boy (docstun) commented Oct 30, 2023

Uh oh!

Hassan Ahmed (hahmed-dev) commented Oct 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Hassan Ahmed (hahmed-dev) commented Oct 12, 2023 •

edited

Loading

Hassan Ahmed (hahmed-dev) Oct 13, 2023 •

edited

Loading

Hassan Ahmed (hahmed-dev) Oct 13, 2023 •

edited

Loading