feat: [OpenAPI] Part 2: Testing Apache Client Support and some fixes. #1062
Conversation
- no more shared client in *Api.java instances
… apiclient package
- *Api.invoke method bugged and unused
- Remove debugging, connectionTimeout, defaultHeaderMap, defaultCookieMap - add TODOs - public setBasePath - Remove addDefaultHeader, setUserAgent,
- Make as manny methods static - Remove deprecated getStatusCode and getResponseHeaders - public parameterToPairs
- Make as many methods static - rename ApiClientResponseHandler to DefaultApiResponseHandler - finish up ApiClient todos
- Enhance JavaDoc - Support Vendor extension `x-sap-cloud-sdk-operation-name`
- Method overloading for required and options param in operations
- final on local vars
…-apache-templates
…pi/test-apache-client-templates
- Hide response body from exception message - regenerate with formatting adjustments - Non-null response when return type is OpenApiResponse. - invokeAPI is nullable
- Make getter for basePath
rpanackal
changed the base branch from
main
to
feat/openapi/refactor-apache-templates
…templates' into feat/openapi/test-apache-client-templates # Conflicts: # datamodel/openapi/openapi-core/src/test/java/com/sap/cloud/sdk/services/openapi/genericreturntype/GenericReturnTypeTest.java
…r getter ignore behaviour
- Make ApiClient almost `@Value` - fix url building with "//" - Fix vulnerabilities of payload leak in exception messages
| * @throws OpenApiRequestException | ||
| * if an error occurs while attempting to invoke the API | ||
| */ | ||
| @Nonnull |
There was a problem hiding this comment.
This @Nonull annotaion is a lie. The same exists in Spring equivalent.
I am assuming this was intentional to not force handling possible null value at the caller. Still, would like some confirmation.
To add some details, invokeAPI() calls will return null when response body is empty or status is. 204
…factor-apache-templates
…pi/test-apache-client-templates
| { | ||
| return Stream | ||
| .of( | ||
| Arguments.of((Function<HttpDestination, Object>) ( des ) -> new TestSpringApi(des).testMethod()), |
| }); | ||
|
|
||
| // Always disable supportUrlQuery as it's not compatible with interface generation | ||
| result.put(SUPPORT_URL_QUERY, "false"); |
There was a problem hiding this comment.
(Question)
This looks surprising. Will this not result in a breaking change? What is this flag doing?
There was a problem hiding this comment.
This flag adds a method like toUrlQueryString() onto all model classes. Basically, it serializes a model class's instance into a output to be appended on the url of GET request. I don't recall the detals anymore.
The conversion to url string involves a chain of toUrlQueryString() invocation for each field. But some fields may be of interface type, leading to compilation error.
The feature supportUrlQuery is specific to only apache and native client. So it doesn't break any expectation for existing spring users.
| //TODO: support byte[] for files? Do via review | ||
| // final byte[] result = sut.sodasDownloadIdGet(1L); | ||
| // assertThat(result).isNotNull(); | ||
| // assertThat(result).isEqualTo(binaryData); |
There was a problem hiding this comment.
(Minor)
We may need an additional unit test for that(?)
| @Test | ||
| void testPutPayload() | ||
| { | ||
| // TODO: discuss whether to ignore null on serialization? Do via review |
There was a problem hiding this comment.
(Minor)
I feel like this is a risky behavior change. While personally I would like that, I'm afraid that users may be affected unwillingly by that. Maybe we can instead come up with a convenience toggle/method somehow.
For comparison, the payload from legacy RestTemplate-based ("Spring") ApiClient:
expected = """
{
"name": "Cola",
"brand": "Coca-Cola",
"quantity": 100,
"packaging" : null,
"price": 1.5,
"id": 0
}
""";
There was a problem hiding this comment.
I changed the serialization behaviour to include null as well.
That said, users can always do ApiClient.withObjectMapper to set a mapper of their preferred configuration. So I don't feel we need to introduce a convenience toggle for the same.
…factor-apache-templates # Conflicts: # datamodel/openapi/openapi-api-apache-sample/pom.xml # datamodel/openapi/openapi-api-apache-sample/src/main/java/com/sap/cloud/sdk/datamodel/openapi/apache/sample/api/OrdersApi.java # datamodel/openapi/openapi-api-apache-sample/src/main/java/com/sap/cloud/sdk/datamodel/openapi/apache/sample/api/SodasApi.java # datamodel/openapi/openapi-core/src/main/java/com/sap/cloud/sdk/services/openapi/apache/ApiClient.java # datamodel/openapi/openapi-core/src/main/java/com/sap/cloud/sdk/services/openapi/apache/BaseApi.java # datamodel/openapi/openapi-core/src/main/java/com/sap/cloud/sdk/services/openapi/apache/DefaultApiResponseHandler.java # datamodel/openapi/openapi-core/src/main/java/com/sap/cloud/sdk/services/openapi/apache/Pair.java
…pi/test-apache-client-templates # Conflicts: # datamodel/openapi/openapi-api-apache-sample/src/main/java/com/sap/cloud/sdk/datamodel/openapi/apache/petstore/api/DefaultApi.java # datamodel/openapi/openapi-api-apache-sample/src/main/java/com/sap/cloud/sdk/datamodel/openapi/apache/petstore/model/ErrorModel.java # datamodel/openapi/openapi-api-apache-sample/src/main/java/com/sap/cloud/sdk/datamodel/openapi/apache/petstore/model/Pet.java # datamodel/openapi/openapi-api-apache-sample/src/main/java/com/sap/cloud/sdk/datamodel/openapi/apache/petstore/model/PetInput.java # datamodel/openapi/openapi-api-apache-sample/src/main/java/com/sap/cloud/sdk/datamodel/openapi/apache/sodastore/model/ColaBarCode.java
Context
SAP/cloud-sdk-java-backlog#464.
Helpful Links:
In the previous PR, we introduced OpenAPI generator support for apache-httpclient template library, effectively making Spring options.
In this PR, there exists the test coverage for the new components equivalent to Spring.
Feature scope:
openapi-coreopenapi-api-apache-samplemoduleopenapi-generator<supportUrlQuery>false</supportUrlQuery>by hard coding it into the generatorDefinition of Done
Error handling created / updated & covered by the tests aboveDocumentation updatedRelease notes updated