ballerina-platform / ballerina-library

The Ballerina Library
https://ballerina.io/learn/api-docs/ballerina/
Apache License 2.0
136 stars 64 forks source link

Enhance generated record name sanitisation for OAS schema name #6578

Closed lnash94 closed 3 months ago

lnash94 commented 5 months ago

Description: We have identified several component schema names in the provided OpenAPI Specification (OAS) that are not user-friendly. These names are difficult to read and understand, which can lead to confusion and inefficiency for developers who interact with the OAS. This issue needs to be addressed to improve the clarity and usability of our API documentation.

Examples:

Impact:

Proposed Solution:

  1. Review all component schema names in the OAS.
  2. Rename schemas to more descriptive and intuitive names that clearly reflect their content and purpose.
  3. Update all references to these schemas throughout the OAS to ensure consistency.
lnash94 commented 5 months ago

This is hold due to other priorities task

ayeshLK commented 4 months ago

While working on the stripe connector it was observed that the generated client build failed with the following compilation error:

ERROR [client.bal:(476:144,476:151)] a function with a non-'external' function body cannot be a dependently-typed function
ERROR [client.bal:(476:144,476:151)] invalid parameter reference: expected 'typedesc', found 'string'
ERROR [client.bal:(480:16,480:57)] incompatible type for parameter 'targetType' with inferred typedesc value: expected 'typedesc<(ballerina/http:2.11.2:Response|anydata)>', found 'typedesc<other>'

The referred client code is as follows:

    resource isolated function get v1/accounts/[string account](map<string|string[]> headers = {}, *GetAccountsAccountQueries queries) returns account|error {
        string resourcePath = string `/v1/accounts/${getEncodedUri(account)}`;
        map<Encoding> queryParamEncoding = {"expand": {style: DEEPOBJECT, explode: true}};
        resourcePath = resourcePath + check getPathForQueryParam(queries, queryParamEncoding);
        return self.clientEp->get(resourcePath, headers);
    }

And it seems like the generated client method has a path-parameter which has the same name as the return type (account) and because of that this build failure occurs.

I think this issue can be fixed by properly sanitizing the type names by also validating against the parameter names. @lnash94 / @TharmiganK can we get this improvement prioritized so that we don't have to manually update the generated code.

Following is a OpenAPI spec [1] which has this parameter naming conflicts.

[1] - https://drive.google.com/file/d/15M2qUYG3MkvaY9o-kiH5YL5EpbXn14II/view?usp=sharing

lnash94 commented 4 months ago

@ayeshLK, With the tool, we currently generate the client using the user-provided OAS. With this improvement, we aim to align name validation with Ballerina naming conventions. We will provide an enhancement for sanitizing the type schema name. Until this fix is available, as a workaround, could you please modify only a particular schema name to Account, which we are supposed to provide as a fix? This will be addressed in the U10 release.

lnash94 commented 4 months ago

Date: 2024/07/16

Screenshot 2024-07-17 at 12 14 56
Example
After modification (Solution) comment
When the DuplicateName Ex: pet_details PetDetails Warning with given name: PetDetails_1
Path parameter has name :  “200” param200  
The type has name: “200” Schema200  
Inline_response_200 InlineResponse200  
200$ record: schema200, path parameter: param200   
@client$Name   record: ClientName , path parameter: clientName  
“” Return the error  
parameter and the type have the same name  query parameter name we can not change, first priority to change schema with Ballerina rules    

non-goal items:

lnash94 commented 3 months ago

We decided to give this feature by enabling a flag for the --use-sanitized-oas.

lnash94 commented 3 months ago

Close the favour with https://github.com/ballerina-platform/openapi-tools/pull/1743