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.

Sign up for GitHub

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

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

Merged
merged 11 commits into from
Oct 30, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
453eb65
TSI-2083 update java client generation file
hahmed-dev Oct 11, 2023
244d233
tsi-2083 update test file
hahmed-dev Oct 12, 2023
df4e052
testing out changes with different string formation
hahmed-dev Oct 12, 2023
1a61838
Add typecasting workaround to test build
hahmed-dev Oct 12, 2023
718bd6d
Updated assertion to match encoded params
hahmed-dev Oct 12, 2023
85ae2fc
try improving gradle test output
jablan Oct 12, 2023
9078009
try improving gradle test output
jablan Oct 12, 2023
b70c0df
try improving gradle test output
jablan Oct 12, 2023
cf52973
Remove redundant line
hahmed-dev Oct 12, 2023
29770d2
Adds ability to parse and embed nested params
hahmed-dev Oct 13, 2023
5f66775
Add deep object directive
hahmed-dev Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/test-java.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ jobs:
with:
distribution: temurin
java-version: 11
- name: Run Gradle Tests
- name: Run gradle test
uses: gradle/gradle-build-action@v2
with:
gradle-version: 7.1.1
build-root-directory: ./clients/java
arguments: test
arguments: test --info
14 changes: 12 additions & 2 deletions clients/java/src/test/java/com/phrase/client/api/LocalesApiTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,15 @@ public void localeDownloadTest() throws ApiException, IOException, InterruptedEx

mockBackend.enqueue(mockResponse);


Map<String, String> nestedFormatOptionsMap = new HashMap<>();
nestedFormatOptionsMap.put("nested_option", "sub_option");

Map<String, Object> formatOptionsMap = new HashMap<>();
formatOptionsMap.put("omit_separator_space", "true");
formatOptionsMap.put("fallback_language", "en");
formatOptionsMap.put("more_options", nestedFormatOptionsMap);

String projectId = "MY_PROJECT_ID";
String id = "MY_ID";
String xPhraseAppOTP = null;
Expand All @@ -151,7 +160,7 @@ public void localeDownloadTest() throws ApiException, IOException, InterruptedEx
Boolean includeTranslatedKeys = null;
Boolean keepNotranslateTags = null;
Boolean convertEmoji = null;
Object formatOptions = null;
Object formatOptions = formatOptionsMap;
String encoding = null;
Boolean skipUnverifiedTranslations = null;
Boolean includeUnverifiedTranslations = null;
Expand All @@ -164,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", recordedRequest.getPath());
// for some reason with deep nested query params, ordering of query params change
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Assert.assertEquals("Request path", "//projects/MY_PROJECT_ID/locales/MY_ID/download?format_options%5Bomit_separator_space%5D=true&format_options%5Bmore_options%5D%5Bnested_option%5D=sub_option&format_options%5Bfallback_language%5D=en", recordedRequest.getPath());
}

/**
Expand Down
4 changes: 3 additions & 1 deletion doc/compiled.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7450,7 +7450,9 @@
"schema": {
"type": "object",
"properties": {}
}
},
"style": "deepObject",
"explode": true
hahmed-dev marked this conversation as resolved.
Show resolved Hide resolved
},
{
"description": "Enforces a specific encoding on the file contents. Valid options are \"UTF-8\", \"UTF-16\" and \"ISO-8859-1\".",
Expand Down
8 changes: 7 additions & 1 deletion openapi-generator/templates/java/build.gradle.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ if(hasProperty('target') && target == 'android') {
main = System.getProperty('mainClass')
classpath = sourceSets.main.runtimeClasspath
}

task sourcesJar(type: Jar, dependsOn: classes) {
classifier = 'sources'
from sourceSets.main.allSource
Expand Down Expand Up @@ -177,6 +177,12 @@ dependencies {
testCompile "junit:junit:$junit_version"
}

test {
testLogging {
outputs.upToDateWhen {false}
showStandardStreams = true
}
}

publishing {
publications {
Expand Down
38 changes: 38 additions & 0 deletions openapi-generator/templates/java/libraries/okhttp-gson/ApiClient.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,27 @@ public class ApiClient {
}
}

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

if(parameter instanceof Map){
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

..and arrays :D

Copy link
Contributor Author

@hahmed-dev hahmed-dev Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Map<String, Object> mappedParameter = (Map<String, Object>) parameter;

for(Map.Entry<String, Object> entry : mappedParameter.entrySet()){
String key = name + "[" + entry.getKey() + "]";
Object value = entry.getValue();

{{! TODO check why nested params ordering is different }}
params.addAll(parsedMappedParams(key, value, new ArrayList<Pair>()));
}

}


return params;
}


/**
* Formats the specified query parameter to a list containing a single {@code Pair} object.
*
Expand Down Expand Up @@ -1445,4 +1466,21 @@ public class ApiClient {
throw new AssertionError(e);
}
}

private List<Pair> parsedMappedParams(String key, Object value, List<Pair> params){
if(value instanceof Map){
Map<String, Object> mappedValue = (Map<String, Object>) value;

for(Map.Entry<String, Object> entry : mappedValue.entrySet()){
String nestedKey = key + "[" + entry.getKey() + "]";
parsedMappedParams(nestedKey, entry.getValue(), params);
}
}
else{
params.add(new Pair(key, parameterToString(value)));
}

return params;
}

}
5 changes: 5 additions & 0 deletions openapi-generator/templates/java/libraries/okhttp-gson/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,12 @@ public class {{classname}} {
{{javaUtilPrefix}}List<Pair> localVarCollectionQueryParams = new {{javaUtilPrefix}}ArrayList<Pair>();
{{#queryParams}}
if ({{paramName}} != null) {
{{#isDeepObject}}
localVarQueryParams.addAll(localVarApiClient.mappedParameterToPairs("{{baseName}}", {{paramName}}));
{{/isDeepObject}}
{{^isDeepObject}}
{{#collectionFormat}}localVarCollectionQueryParams.addAll(localVarApiClient.parameterToPairs("{{{collectionFormat}}}", {{/collectionFormat}}{{^collectionFormat}}localVarQueryParams.addAll(localVarApiClient.parameterToPair({{/collectionFormat}}"{{baseName}}", {{paramName}}));
{{/isDeepObject}}
}

{{/queryParams}}
Expand Down
2 changes: 2 additions & 0 deletions paths/locales/download.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ parameters:
schema:
type: object
properties: {}
style: deepObject
explode: true
hahmed-dev marked this conversation as resolved.
Show resolved Hide resolved
- description: Enforces a specific encoding on the file contents. Valid options are "UTF-8", "UTF-16" and "ISO-8859-1".
example:
name: encoding
Expand Down
Loading