Skip to content

Conversation

@rpanackal
Copy link
Member

@rpanackal rpanackal commented Dec 29, 2025

Context

SAP/cloud-sdk-java-backlog#464.

There are two objectives to this PR

  1. Make Spring dependencies optional by introducing Apache HttpClient reliance.
  2. Serve as base/reference branch to compare all upcoming changes/PRs. This PR contains, all required files and api to support apache-httpclient template based code generation. We will (soon) remove a large chunk of introduced files, reduce public API and even refactor code.

To maintain backward compatibility Spring framework dependencies are marked optional but template files, or any public API are still maintained without any changes.

Feature scope:

  • New package com.sap.cloud.sdk.services.openapi.apache with ApiClient, BaseApi and other supporting classes added
  • New openapi-api-apache-sample module as e2e test with sample code generation with apache/ApiClient

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

@rpanackal rpanackal force-pushed the feat/openapi/optional-spring-base branch from f47b1db to cde369f Compare December 30, 2025 09:26
public class OrdersApi extends BaseApi
{

public OrdersApi()
Copy link
Contributor

Choose a reason for hiding this comment

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

(Major)

Destination constructor missing

* @throws ApiException
* if fails to make API call
*/
public Order ordersPost( @javax.annotation.Nonnull Order order )
Copy link
Contributor

Choose a reason for hiding this comment

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

(Question/Optional)

It may be worth our time to check for alternatives to javax.annotation.Nonnull null-indicator annotations.

}

@Override
public <T> T invokeAPI(
Copy link
Contributor

Choose a reason for hiding this comment

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

(Major)

I think we would prefer not to have this method public. (for now)

* @throws ApiException
* if fails to make API call
*/
public File sodasDownloadIdGet( @javax.annotation.Nonnull Long id, Map<String, String> additionalHeaders )
Copy link
Contributor

Choose a reason for hiding this comment

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

(Major)

I think we would prefer not have additional (public) headers-method.

* @throws ApiException
* if fails to make API call
*/
public File sodasDownloadIdGet( @javax.annotation.Nonnull Long id )
Copy link
Contributor

Choose a reason for hiding this comment

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

(Question/Minor)

For better comparison, would you consider offering your byte[] fix here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Additionally)
The method is not returning @Nonnull annotation on result (?)

* @throws ApiException
* if fails to make API call
*/
public List<SodaWithId> sodasGet()
Copy link
Contributor

Choose a reason for hiding this comment

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

(Minor)

Missing @Nonnull or is this deliberate

}
return o.toString().replace("\n", "\n ");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

(Major)

Your model classes are missing static factory method(s)? Is this deliberate - or unwanted side-effect?

public static Fanta create()
    {
        return new Fanta();
    }

return apiClient;
}

public void setApiClient( ApiClient apiClient )
Copy link
Contributor

@newtork newtork Dec 30, 2025

Choose a reason for hiding this comment

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

(Major)

We do not want mutable api-client (for now).
All the other methods of this class need some alignment too.

@newtork newtork added the do not merge Pull request must not be merged label Dec 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Pull request must not be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants