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.

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

[windows] Fixed missing output #19715

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/openapi-generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ jobs:
path: modules/openapi-generator-cli/target
- name: Delete samples that are entirely generated
run: |
rm -rf samples/client/petstore/csharp/generichost/latest/Tags

rm -rf samples/client/petstore/csharp/generichost/net8/AllOf
rm -rf samples/client/petstore/csharp/generichost/net8/AnyOf
rm -rf samples/client/petstore/csharp/generichost/net8/AnyOfNoCompare
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/samples-dotnet.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ name: Samples C# .Net 8 Clients
on:
push:
paths:
- samples/client/petstore/csharp/generichost/latest/**
- samples/client/petstore/csharp/generichost/net8/**
- samples/client/petstore/csharp/httpclient/net8/**
- samples/client/petstore/csharp/restsharp/net8/**
- samples/client/petstore/csharp/unityWebRequest/net8/**
pull_request:
paths:
- samples/client/petstore/csharp/generichost/latest/**
- samples/client/petstore/csharp/generichost/net8/**
- samples/client/petstore/csharp/httpclient/net8/**
- samples/client/petstore/csharp/restsharp/net8/**
Expand All @@ -21,6 +23,7 @@ jobs:
fail-fast: false
matrix:
sample:
- samples/client/petstore/csharp/generichost/latest/Tags
- samples/client/petstore/csharp/generichost/net8/AllOf
- samples/client/petstore/csharp/generichost/net8/AnyOf
- samples/client/petstore/csharp/generichost/net8/AnyOfNoCompare
Expand Down
10 changes: 10 additions & 0 deletions bin/configs/csharp-generichost-tags-latest.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# for csharp generichost
generatorName: csharp
outputDir: samples/client/petstore/csharp/generichost/latest/Tags
inputSpec: modules/openapi-generator/src/test/resources/3_0/csharp/tags.json
library: generichost
templateDir: modules/openapi-generator/src/main/resources/csharp
additionalProperties:
packageGuid: '{321C8C3F-0156-40C1-AE42-D59761FB9B6C}'
modelPropertySorting: alphabetical
operationParameterSorting: alphabetical
Original file line number Diff line number Diff line change
Expand Up @@ -1452,7 +1452,7 @@ public String toRegularExpression(String pattern) {
}

/**
* Return the file name of the Api Test
* Return the file name of the Api
*
* @param name the file name of the Api
* @return the file name of the Api
Expand Down Expand Up @@ -5674,6 +5674,8 @@ protected void addHeaders(ApiResponse response, List<CodegenProperty> properties
}
}

private final Map<String, Integer> seenOperationIds = new HashMap<String, Integer>();

/**
* Add operation to group
*
Expand All @@ -5694,13 +5696,18 @@ public void addOperationToGroup(String tag, String resourcePath, Operation opera
}
// check for operationId uniqueness
String uniqueName = co.operationId;
int counter = 0;
int counter = seenOperationIds.getOrDefault(uniqueName, 0);
while(seenOperationIds.containsKey(uniqueName)) {
uniqueName = co.operationId + "_" + counter;
counter++;
}
Copy link
Member

Choose a reason for hiding this comment

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

looks like this breaks the use case in which more than one operation having the same name (e.g. getId) and now users will get something different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can revert it but we will need a sample for the use case this breaks and another pr with this fix that doesn't break that sample.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say users get something different, is it wrong, or just different?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's nice addition to prevent spec with duplicated operationId generating code that won't compile. Thanks again for the PR.

Let me think about how to deal with auto fix (opt in or out) and come up with a solution.

Will definitely add samples/tests for to cover this case moving forward.

Thanks again for your work!

Copy link
Member

Choose a reason for hiding this comment

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

When you say users get something different, is it wrong, or just different?

please refer to the discussion in https://openapi-generator.slack.com/archives/CLSB0U0R5/p1728581031429579?thread_ts=1728372817.754659&cid=CLSB0U0R5

Copy link
Member

Choose a reason for hiding this comment

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

please take your time

Copy link

@OlliL OlliL Oct 18, 2024

Choose a reason for hiding this comment

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

I have multiple endpoints in my openapi.json which have the same operationId. This was perfectly fine in 7.8.0 and is also fine "in real life" because they are all below different path. I have for example two GET endpoints

  • "/moneyflow/server/monthlysettlement/getAvailableMonth" with operationId "getAvailableMonth"
  • "/moneyflow/server/report/getAvailableMonth" also with operationId "getAvailableMonth"

With the JavaSpring generator, I now get interfaces generated with "getAvailableMonth_0" inside the ReportControllerApi and one "getAvailableMonth" in MonthlySettlementControllerApi.

This is just an example - I have multiple endpoints with "create", "delete" and so on operationIds - they now all end up getting generated as create_0 up to create_4 - all in different Controllers.... this really looks like a mess to be honest.

How can I revert back to the old behaviour. There is no point to make this methods unique project-wide when they end up getting generated in different Controller Interfaces (not generating a client, using tags to group endpoints controller-wise). Additionally those numbers don't seem to be predictable so it just ends up with not usable method names (because they may vary in the future I guess). At least tags should be considered to verify if random numbers have to be added or not. It makes no sense to add them for same operationIds in different tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clearly the _0 is a breaking change. The issue for me is that I use the operationId in the name of a class which means it has to be unique. I thought using the operationId would give me a unique value, but that is not true when there are tags. I'll have to explore a different way to handle this.

Copy link

@OlliL OlliL Oct 18, 2024

Choose a reason for hiding this comment

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

On class level you are right (have to be unique). If I understood the initial problem correctly, it was all about Windows filesystem limitations (case insensitive). So maybe the file names have to be unique case-insensitiive wise, but the code inside the files might not. I mean while Windows can't have File1.x and file1.x in parallel, it might have File1.x and file1_0.x but with classes File1 and file1 inside. But - not sure if there are languages out which dictate classname must be equal filename.

And one problem remains imho. The names become inpredictable. What guarantees that "...._0" will be "..._0" in the future and not silently will be generated as "..._2" at one point instead. Maybe just because some dev decides to reorder the openapi spec? Adding iterating numbers seems not right at all imho but maybe I'm missing something 😄

What about taking the tag into consideration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The _0 part of this pr reverted in a new pr. Thanks for being understanding. #19913

for (CodegenOperation op : opList) {
if (uniqueName.equals(op.operationId)) {
uniqueName = co.operationId + "_" + counter;
counter++;
}
}
seenOperationIds.put(co.operationId, counter);
if (!co.operationId.equals(uniqueName)) {
LOGGER.warn("generated unique operationId `{}`", uniqueName);
}
Expand Down Expand Up @@ -6076,30 +6083,68 @@ protected String removeNonNameElementToCamelCase(final String name, final String
return result;
}

/**
* Return a value that is unique, suffixed with _index to make it unique
* Ensures generated files are unique when compared case-insensitive
* Not all operating systems support case-sensitive paths
*/
private String uniqueCaseInsensitiveString(String value, Map<String, String> seenValues) {
if (seenValues.keySet().contains(value)) {
return seenValues.get(value);
}

Optional<Entry<String,String>> foundEntry = seenValues.entrySet().stream().filter(v -> v.getValue().toLowerCase(Locale.ROOT).equals(value.toLowerCase(Locale.ROOT))).findAny();
if (foundEntry.isPresent()) {
int counter = 0;
String uniqueValue = value + "_" + counter;

while (seenValues.values().stream().map(v -> v.toLowerCase(Locale.ROOT)).collect(Collectors.toList()).contains(uniqueValue.toLowerCase(Locale.ROOT))) {
counter++;
uniqueValue = value + "_" + counter;
}

seenValues.put(value, uniqueValue);
return uniqueValue;
}

seenValues.put(value, value);
return value;
}

private final Map<String, String> seenApiFilenames = new HashMap<String, String>();

@Override
public String apiFilename(String templateName, String tag) {
String uniqueTag = uniqueCaseInsensitiveString(tag, seenApiFilenames);
String suffix = apiTemplateFiles().get(templateName);
return apiFileFolder() + File.separator + toApiFilename(tag) + suffix;
return apiFileFolder() + File.separator + toApiFilename(uniqueTag) + suffix;
}

@Override
public String apiFilename(String templateName, String tag, String outputDir) {
String uniqueTag = uniqueCaseInsensitiveString(tag, seenApiFilenames);
String suffix = apiTemplateFiles().get(templateName);
return outputDir + File.separator + toApiFilename(tag) + suffix;
return outputDir + File.separator + toApiFilename(uniqueTag) + suffix;
}

private final Map<String, String> seenModelFilenames = new HashMap<String, String>();

@Override
public String modelFilename(String templateName, String modelName) {
String uniqueModelName = uniqueCaseInsensitiveString(modelName, seenModelFilenames);
String suffix = modelTemplateFiles().get(templateName);
return modelFileFolder() + File.separator + toModelFilename(modelName) + suffix;
return modelFileFolder() + File.separator + toModelFilename(uniqueModelName) + suffix;
}

@Override
public String modelFilename(String templateName, String modelName, String outputDir) {
String uniqueModelName = uniqueCaseInsensitiveString(modelName, seenModelFilenames);
String suffix = modelTemplateFiles().get(templateName);
return outputDir + File.separator + toModelFilename(modelName) + suffix;
return outputDir + File.separator + toModelFilename(uniqueModelName) + suffix;
}

private final Map<String, String> seenApiDocFilenames = new HashMap<String, String>();

/**
* Return the full path and API documentation file
*
Expand All @@ -6109,11 +6154,14 @@ public String modelFilename(String templateName, String modelName, String output
*/
@Override
public String apiDocFilename(String templateName, String tag) {
String uniqueTag = uniqueCaseInsensitiveString(tag, seenApiDocFilenames);
String docExtension = getDocExtension();
String suffix = docExtension != null ? docExtension : apiDocTemplateFiles().get(templateName);
return apiDocFileFolder() + File.separator + toApiDocFilename(tag) + suffix;
return apiDocFileFolder() + File.separator + toApiDocFilename(uniqueTag) + suffix;
}

private final Map<String, String> seenApiTestFilenames = new HashMap<String, String>();

/**
* Return the full path and API test file
*
Expand All @@ -6123,8 +6171,9 @@ public String apiDocFilename(String templateName, String tag) {
*/
@Override
public String apiTestFilename(String templateName, String tag) {
String uniqueTag = uniqueCaseInsensitiveString(tag, seenApiTestFilenames);
String suffix = apiTestTemplateFiles().get(templateName);
return apiTestFileFolder() + File.separator + toApiTestFilename(tag) + suffix;
return apiTestFileFolder() + File.separator + toApiTestFilename(uniqueTag) + suffix;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1434,6 +1434,8 @@ protected File processTemplateToFile(Map<String, Object> templateData, String te
return processTemplateToFile(templateData, templateName, outputFilename, shouldGenerate, skippedByOption, this.config.getOutputDir());
}

private final Set<String> seenFiles = new HashSet<>();

private File processTemplateToFile(Map<String, Object> templateData, String templateName, String outputFilename, boolean shouldGenerate, String skippedByOption, String intendedOutputDir) throws IOException {
String adjustedOutputFilename = outputFilename.replaceAll("//", "/").replace('/', File.separatorChar);
File target = new File(adjustedOutputFilename);
Expand All @@ -1444,6 +1446,11 @@ private File processTemplateToFile(Map<String, Object> templateData, String temp
if (!absoluteTarget.startsWith(outDir)) {
throw new RuntimeException(String.format(Locale.ROOT, "Target files must be generated within the output directory; absoluteTarget=%s outDir=%s", absoluteTarget, outDir));
}

if (seenFiles.stream().filter(f -> f.toLowerCase(Locale.ROOT).equals(absoluteTarget.toString().toLowerCase(Locale.ROOT))).findAny().isPresent()) {
LOGGER.warn("Duplicate file path detected. Not all operating systems can handle case sensitive file paths. path={}", absoluteTarget.toString());
}
seenFiles.add(absoluteTarget.toString());
return this.templateProcessor.write(templateData, templateName, target);
} else {
this.templateProcessor.skip(target.toPath(), String.format(Locale.ROOT, "Skipped by %s options supplied by user.", skippedByOption));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ using System.Text;
using System.Text.Json;
using System.Text.RegularExpressions;{{#useCompareNetObjects}}
using KellermanSoftware.CompareNetObjects;{{/useCompareNetObjects}}
{{#models}}
{{#-first}}
using {{packageName}}.{{modelPackage}};
{{/-first}}
{{/models}}
using System.Runtime.CompilerServices;

{{>Assembly}}namespace {{packageName}}.{{clientPackage}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ using System.Text.Json.Serialization;
using System.Net.Http;
using Microsoft.Extensions.DependencyInjection;
using {{packageName}}.{{apiPackage}};
{{#models}}
{{#-first}}
using {{packageName}}.{{modelPackage}};
{{/-first}}
{{/models}}

namespace {{packageName}}.{{clientPackage}}
{
Expand Down
82 changes: 82 additions & 0 deletions modules/openapi-generator/src/test/resources/3_0/csharp/tags.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
{
"info": {
"title": "Files.com API",
"contact": {
"name": "Files.com Customer Success Team",
"email": "[email protected]"
},
"version": "0.0.1"
},
"swagger": "2.0",
"produces": [
"application/json",
"application/msgpack",
"application/xml"
],
"securityDefinitions": {
"api_key": {
"type": "apiKey",
"description": "API Key - supports user-based or site-wide API keys",
"name": "XFilesAPIKey",
"in": "header"
}
},
"host": "app.files.com",
"basePath": "/api/rest/v1",
"tags": [
{
"name": "api_key",
"description": "Operations about api_keys"
},
{
"name": "API Keys",
"description": "Operations about API Keys"
},
{
"name": "a_p_i_k_e_y_s",
"description": "Operations about API keys"
}
],
"paths": {
"/api_keys/{id}": {
"get": {
"summary": "Show API Key",
"description": "Show API Key",
"produces": [
"application/json"
],
"parameters": [
{
"in": "path",
"name": "id",
"description": "Api Key ID.",
"type": "integer",
"format": "int32",
"required": true,
"x-ms-summary": "Api Key ID."
}
],
"responses": {
"400": {
"description": "Bad Request",
"x-ms-summary": "Bad Request"
}
},
"tags": [
"api_keys",
"API Keys",
"a_p_i_k_e_y_s"
devhl-labs marked this conversation as resolved.
Show resolved Hide resolved
],
"operationId": "GetApiKeysId",
"x-authentication": [
"self_managed"
],
"x-category": [
"developers"
]
}
}
}
,
"definitions": {}
}
Loading
Loading