Conversation
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <project.build.outputTimestamp>2025-04-03T13:23:00Z</project.build.outputTimestamp> | ||
| <cloud-sdk.version>5.26.0</cloud-sdk.version> | ||
| <cloud-sdk.version>5.27.0-SNAPSHOT</cloud-sdk.version> |
There was a problem hiding this comment.
I guess you need to remove snapshot once cloud sdk is released? So we merge cloud sdk PR first?
| return apiClient.invokeAPI( | ||
| localVarPath, | ||
| "POST", | ||
| localVarQueryParams, | ||
| localVarCollectionQueryParams, | ||
| localVarQueryStringJoiner.toString(), | ||
| requestBody, | ||
| localVarHeaderParams, | ||
| localVarFormParams, | ||
| localVarAccept, | ||
| localVarContentType, | ||
| localVarReturnType); |
There was a problem hiding this comment.
This method.. crazy. It reminds me of some Windows API which requires a long list of params. Since this method is still marked with @beta and is public to end-user, I strongly recommend to redesign it.
For example, consider builder pattern to help skip nullable params.
RequestBuilder requestBuilder = new RequestBuilder();
builder
.setPath('/abc')
.setMethod('POST')
.setXYZ()
.invoke(); // Consider put the request body inside invoke()Also why is formParams and headerParams Nonnull? They can for sure be null (or by default set to empty Map?)
There was a problem hiding this comment.
The ApiClient is straight from OpenAPI Generator. We shouldn't put any effort towards generated code or modified code from OpenAPI.
There was a problem hiding this comment.
@ZhongpinWang We agree that the API in question has too many parameters and ideally redesigned. But, I would like to add some historic context to this.
- Originally ApiClient is a generated class but once you stabilise the choice of client library (eg: apache, spring etc), there is no need to generate it per spec. So, we copied the class and packaged it into the Cloud SDK OpenAPI Generator. This also meant we could customise it as needed.
- The code quality of the ApiClient is highly questionable. But, we limited any refactoring effort to simply improve readability, atleast on a higher level and reduce public api surface. This was also done to make reviewing the original PR easier (~20k lines of changes).
We can, if needed, create BLIs to improve the code quality here. Its not clear how much ownership of this we want to assume atm.
Jonas-Isr
left a comment
Jonas-Isr
left a comment
There was a problem hiding this comment.
Code looks good so far (besides the snapshot in the pomp course). But should we not test this? At least the happy path?
It's already tested in |
Jonas-Isr
left a comment
Jonas-Isr
left a comment
There was a problem hiding this comment.
Code itself looks good. The thing I am wondering is: IIRC the JS team showed in the demo that they only compress large input, right? And that the user has a choice to disable compression? Is there a reason you did not consider that? And what is your estimate on how breaking it would be to add a choice for the user in the future?
| public PredictResponsePayload tableCompletion(@Nonnull final PredictRequestPayload requestBody) | ||
| throws OpenApiRequestException { | ||
|
|
||
| // create path and map variables | ||
| final String localVarPath = "/predict"; | ||
|
|
||
| final StringJoiner localVarQueryStringJoiner = new StringJoiner("&"); | ||
| final List<Pair> localVarQueryParams = new ArrayList<>(); | ||
| final List<Pair> localVarCollectionQueryParams = new ArrayList<>(); | ||
| final Map<String, String> localVarHeaderParams = Map.of("Content-Encoding", "gzip"); | ||
| final Map<String, Object> localVarFormParams = new HashMap<>(); | ||
|
|
||
| final String[] localVarAccepts = {"application/json"}; | ||
| final String localVarAccept = ApiClient.selectHeaderAccept(localVarAccepts); | ||
| final String[] localVarContentTypes = {"application/json"}; | ||
| final String localVarContentType = ApiClient.selectHeaderContentType(localVarContentTypes); | ||
|
|
||
| final TypeReference<PredictResponsePayload> localVarReturnType = new TypeReference<>() {}; | ||
|
|
||
| return apiClient.invokeAPI( |
There was a problem hiding this comment.
(major)
The current approach has a major defect that you are skipping the predict method in DefaultApi class - which is generated and importantly, will be in sync with any spec updates. So, once there are updates to the spec file, how would you guarantee that the manually written code here will be in sync? What happens if there was a breaking api change, how would you expect to catch in your SDK Code Generation workflow?
So, I am struggling to get onboard with this design.
Ideally, the gzip feature should be fully developed as an opt-in feature of Cloud SDK Generator, with possibly updates to the template files. I agree that there may be additional complexity involved w.r.t to having a feature toggle per operation since we may not want gzipping enabled for all operations.
There was a problem hiding this comment.
Ok I will try to find a way to have it only in Cloud SDK
4 participants
Context
AI/ai-sdk-java-backlog#363.
Smaller payload on SAP RPT.
Corresponding Cloud SDK PR:
Feature scope:
Definition of Done