Skip to content

Conversation

@rpanackal
Copy link
Member

@rpanackal rpanackal commented Jan 7, 2026

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:

  • Unit testing in openapi-core
  • Test serialization, deserialization and oneOf support in openapi-api-apache-sample module
  • Integration testing in openapi-generator
  • Pin <supportUrlQuery>false</supportUrlQuery> by hard coding it into the generator

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Documentation updated
  • Release notes updated

- no more shared client in *Api.java instances
- *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
- Hide response body from exception message
- regenerate with formatting adjustments
- Non-null response when return type is OpenApiResponse.
- invokeAPI is nullable
- 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
Copy link
Member Author

@rpanackal rpanackal Jan 7, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Because there is no chance we get a null on anything but a 204

@rpanackal rpanackal self-assigned this Jan 8, 2026
@rpanackal rpanackal added the please review Request to review a pull request label Jan 8, 2026
});

// Always disable supportUrlQuery as it's not compatible with interface generation
result.put(SUPPORT_URL_QUERY, "false");
Copy link
Contributor

Choose a reason for hiding this comment

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

(Question)

This looks surprising. Will this not result in a breaking change? What is this flag doing?

Copy link
Member Author

@rpanackal rpanackal Jan 12, 2026

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

(Minor)

We may need an additional unit test for that(?)

Copy link
Member Author

Choose a reason for hiding this comment

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

We do have support for byte[], unlike what I initially though was the case. So I uncommented the test here.

Base automatically changed from feat/openapi/refactor-apache-templates to feat/openapi/optional-spring-base January 12, 2026 09:42
…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
…st-apache-client-templates

# Conflicts:
#	datamodel/openapi/openapi-generator/src/main/resources/openapi-generator/mustache-templates/libraries/apache-httpclient/api.mustache
…st-apache-client-templates

# Conflicts:
#	datamodel/openapi/openapi-generator/src/main/resources/openapi-generator/mustache-templates/libraries/apache-httpclient/api.mustache
…st-apache-client-templates

# Conflicts:
#	datamodel/openapi/openapi-generator/src/main/resources/openapi-generator/mustache-templates/libraries/apache-httpclient/operationBody.mustache
@rpanackal rpanackal merged commit 859a8f1 into feat/openapi/optional-spring-base Jan 15, 2026
3 checks passed
@rpanackal rpanackal deleted the feat/openapi/test-apache-client-templates branch January 15, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

please review Request to review a pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants