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

[protobuf-schema] Replace zero value enum suffix from UNKNOWN to UNSPECIFIED #20473

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+20 −20
Open

[protobuf-schema] Replace zero value enum suffix from UNKNOWN to UNSPECIFIED #20473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d71328
enum zero index is UNSPECIFIED suffix
luckyxilu66 Dec 30, 2024
547556b
Merge branch 'OpenAPITools:master' into enum
lucy66hw Jan 18, 2025
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
8 changes: 4 additions & 4 deletions docs/generators/protobuf-schema.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ title: Documentation for the protobuf-schema Generator
## CONFIG OPTIONS
These options may be applied as additional-properties (cli) or configOptions (plugins). Refer to [configuration docs](https://openapi-generator.tech/docs/configuration) for more details.

| Option | Description | Values | Default |
| ------ | ----------- | ------ | ------- |
|numberedFieldNumberList|Field numbers in order.| |false|
|startEnumsWithUnknown|Introduces "UNKNOWN" as the first element of enumerations.| |false|
| Option | Description | Values | Default |
|---------------------------|--------------------------------------------------------------------------| ------ | ------- |
| numberedFieldNumberList | Field numbers in order. | |false|
| startEnumsWithUnspecified | Introduces "UNSPECIFIED" as the first element of enumerations. | |false|

## IMPORT MAPPING

Expand Down
32 changes: 16 additions & 16 deletions ...api-generator/src/main/java/org/openapitools/codegen/languages/ProtobufSchemaCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,15 @@ public class ProtobufSchemaCodegen extends DefaultCodegen implements CodegenConf

public static final String NUMBERED_FIELD_NUMBER_LIST = "numberedFieldNumberList";

public static final String START_ENUMS_WITH_UNKNOWN = "startEnumsWithUnknown";
public static final String START_ENUMS_WITH_UNSPECIFIED = "startEnumsWithUnspecified";

private final Logger LOGGER = LoggerFactory.getLogger(ProtobufSchemaCodegen.class);

@Setter protected String packageName = "openapitools";

private boolean numberedFieldNumberList = false;

private boolean startEnumsWithUnknown = false;
private boolean startEnumsWithUnspecified = false;

@Override
public CodegenType getTag() {
Expand Down Expand Up @@ -164,7 +164,7 @@ public ProtobufSchemaCodegen() {
cliOptions.clear();

addSwitch(NUMBERED_FIELD_NUMBER_LIST, "Field numbers in order.", numberedFieldNumberList);
addSwitch(START_ENUMS_WITH_UNKNOWN, "Introduces \"UNKNOWN\" as the first element of enumerations.", startEnumsWithUnknown);
addSwitch(START_ENUMS_WITH_UNSPECIFIED, "Introduces \"UNSPECIFIED\" as the first element of enumerations.", startEnumsWithUnspecified);
}

@Override
Expand Down Expand Up @@ -195,8 +195,8 @@ public void processOpts() {
this.numberedFieldNumberList = convertPropertyToBooleanAndWriteBack(NUMBERED_FIELD_NUMBER_LIST);
}

if (additionalProperties.containsKey(this.START_ENUMS_WITH_UNKNOWN)) {
this.startEnumsWithUnknown = convertPropertyToBooleanAndWriteBack(START_ENUMS_WITH_UNKNOWN);
if (additionalProperties.containsKey(this.START_ENUMS_WITH_UNSPECIFIED)) {
this.startEnumsWithUnspecified = convertPropertyToBooleanAndWriteBack(START_ENUMS_WITH_UNSPECIFIED);
}

supportingFiles.add(new SupportingFile("README.mustache", "", "README.md"));
Expand Down Expand Up @@ -233,6 +233,7 @@ public void addEnumValuesPrefix(Map<String, Object> allowableValues, String pref
String name = (String) value.get("name");
value.put("name", prefix + "_" + name);
value.put("value", "\"" + prefix + "_" + name + "\"");

}
}

Expand All @@ -249,22 +250,21 @@ public void addEnumValuesPrefix(Map<String, Object> allowableValues, String pref
*
* @param allowableValues allowable values
*/
public void addUnknownToAllowableValues(Map<String, Object> allowableValues) {
if (startEnumsWithUnknown) {
public void addUnspecifiedToAllowableValues(Map<String, Object> allowableValues) {
Copy link

Choose a reason for hiding this comment

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

@lucy66hw just a question please, reading the https://protobuf.dev/best-practices/dos-donts/#unspecified-enum. it seems like the right name for first element should have UNSPECIFIED suffix, fe <enum_name>_UNSPECIFIED. Shouldn't we follow this convention? Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

@reta thanks for comment. yes, this is the target of the PR. It replaces the current zero enum suffix from UNKNOWN to UNSPECIFIED. The enum_name prefix will be added by function addEnumValuesPrefix, resulting in the zero enum being named <enum_name>_UNSPECIFIED

if (startEnumsWithUnspecified) {
if (allowableValues.containsKey("enumVars")) {
List<Map<String, Object>> enumVars = (List<Map<String, Object>>) allowableValues.get("enumVars");

HashMap<String, Object> unknown = new HashMap<String, Object>();
unknown.put("name", "UNKNOWN");
unknown.put("isString", "false");
unknown.put("value", "\"UNKNOWN\"");

enumVars.add(0, unknown);
HashMap<String, Object> unspecified = new HashMap<String, Object>();
unspecified.put("name", "UNSPECIFIED");
unspecified.put("isString", "false");
unspecified.put("value", "\"UNSPECIFIED\"");
enumVars.add(0, unspecified);
}

if (allowableValues.containsKey("values")) {
List<String> values = (List<String>) allowableValues.get("values");
values.add(0, "UNKNOWN");
values.add(0, "UNSPECIFIED");
}
}
}
Expand All @@ -291,7 +291,7 @@ public ModelsMap postProcessModels(ModelsMap objs) {

if (cm.isEnum) {
Map<String, Object> allowableValues = cm.getAllowableValues();
addUnknownToAllowableValues(allowableValues);
addUnspecifiedToAllowableValues(allowableValues);
addEnumValuesPrefix(allowableValues, cm.getClassname());
if (allowableValues.containsKey("enumVars")) {
List<Map<String, Object>> enumVars = (List<Map<String, Object>>) allowableValues.get("enumVars");
Expand Down Expand Up @@ -319,7 +319,7 @@ public ModelsMap postProcessModels(ModelsMap objs) {
}

if (var.isEnum) {
addUnknownToAllowableValues(var.allowableValues);
addUnspecifiedToAllowableValues(var.allowableValues);
addEnumValuesPrefix(var.allowableValues, var.getEnumName());

if (var.allowableValues.containsKey("enumVars")) {
Expand Down