Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[OpenAPI Generator] Enable allow-list for API and Model classes #530

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[OpenAPI Generator] Enable allow-list for API and Model classes #530

wants to merge 5 commits into from

Conversation

newtork
Copy link
Contributor

@newtork newtork commented Aug 9, 2024 •
edited
Loading

Please find similar setting names in the original OpenAPI generator.

Usage:

  • In additionalProperties define modelsToGenerate and apisToGenerate. Use <space> or , as delimiter.
<plugin>
  <groupId>com.sap.cloud.sdk.datamodel</groupId>
  <artifactId>openapi-generator-maven-plugin</artifactId>
  <configuration>
    <additionalProperties>
      <apisToGenerate>Sodas,Orders</apisToGenerate>
      <modelsToGenerate>
        Order
        OrderWithTimestamp
        Soda
        SodaWithId
      </modelsToGenerate>
    </additionalProperties>

Note:

  • To enable the generation of file SodasApi.java, the apisToGenerate field should contain Sodas (without suffix),
  • If apisToGenerate is not defined, all APIs are allowed. Same with modelsToGenerate.

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

@newtork newtork added do not merge Pull request must not be merged please review Request to review a pull request labels Aug 9, 2024
@newtork newtork marked this pull request as draft August 9, 2024 19:23
@newtork newtork mentioned this pull request Aug 14, 2024
Closed
8 tasks
@newtork
Copy link
Contributor Author

newtork commented Sep 23, 2024 •
edited
Loading

Moved stale PR to personal fork.

@newtork newtork closed this Sep 23, 2024
@newtork newtork reopened this Dec 17, 2024
a-d added 2 commits December 17, 2024 11:33
@a-d
Merge remote-tracking branch 'origin/main' into openapi/allowlist-api…
1bbc455 
…s-and-models

# Conflicts:
#	datamodel/openapi/openapi-generator/src/main/java/com/sap/cloud/sdk/datamodel/openapi/generator/GenerationConfigurationConverter.java
@a-d
Format
36955cf
@newtork newtork marked this pull request as ready for review December 17, 2024 10:34
@newtork newtork added please merge Request to merge a pull request and removed do not merge Pull request must not be merged labels Dec 17, 2024
MatKuhr
MatKuhr approved these changes Dec 18, 2024
View reviewed changes
Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

generally lgtm. However, allow-list is not ideal for our purposes. E.g. for orchestration we already have 47 files, and typically we would want to only exclude a few files from generation.

Did you check the options for block-list? Maybe using .openapi-generator-ignore can be made to work?

...ain/java/com/sap/cloud/sdk/datamodel/openapi/generator/GenerationConfigurationConverter.java
@@ -115,6 +115,15 @@ private static void setGlobalSettings( @Nonnull final GenerationConfiguration co
GlobalSettings.setProperty(CodegenConstants.HIDE_GENERATION_TIMESTAMP, Boolean.TRUE.toString());
}

private static String getAllowedIds( @Nonnull final GenerationConfiguration config, @Nonnull final String property )
Copy link
Member

Choose a reason for hiding this comment

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

(minor) a unit test for this would be nice, that would also clarify that both whitespaces and newlines are supported as separators

...st/java/com/sap/cloud/sdk/datamodel/openapi/generator/DataModelGeneratorIntegrationTest.java
true,
true,
3,
ofEntries(entry("apisToGenerate", "Default, NotExist"), entry("modelsToGenerate", "Cola,Fanta ,NotExist")));
Copy link
Member

Choose a reason for hiding this comment

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

(question) is this intentional, or should it be:

Suggested change
ofEntries(entry("apisToGenerate", "Default, NotExist"), entry("modelsToGenerate", "Cola,Fanta ,NotExist")));
ofEntries(entry("apisToGenerate", "Default, NotExist"), entry("modelsToGenerate", "Cola, Fanta, NotExist")));

...or/src/test/resources/DataModelGeneratorIntegrationTest/generate-models/input/sodastore.yaml
version: 1.0.0
description: API for managing sodas in a soda store
paths:
/sodas:
Copy link
Member

Choose a reason for hiding this comment

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

(minor) since there is only one API, this only tests that NotExist doesn't cause the generation to fail..

...or/src/test/resources/DataModelGeneratorIntegrationTest/generate-models/input/sodastore.yaml
$ref: '#/components/schemas/OneOf'
components:
schemas:
OneOfWithDiscriminatorAndMapping:
Copy link
Member

Choose a reason for hiding this comment

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

(minor) the following would prevent finding unrelated tests when searching our code base for oneOf:

Suggested change
OneOfWithDiscriminatorAndMapping:
ShouldNotBeGenerated:

@newtork newtork added please review Request to review a pull request and removed please review Request to review a pull request please merge Request to merge a pull request labels Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MatKuhr MatKuhr MatKuhr approved these changes

Assignees
No one assigned
Labels
please review Request to review a pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@newtork @MatKuhr @a-d