Protect object property & enum member names

[dickermoshe]

No description provided.

[dickermoshe]

@walsha2
What is going on here, I understand that it's renaming the names of enum mumbers, but what is this empty , unknown...?

openapi_spec/lib/src/generators/schema.dart

Lines 964 to 987 in e4bf912

	String _safeEnumValue(String value, Schema schema) {
	// Dart enums cannot start with a number
	if (value.startsWith(RegExp(r'[0-9]'))) {
	value = 'v$value';
	}
	value = value.replaceAll('.', '_').camelCase;
	if (value.isEmpty) {
	schema.mapOrNull(enumeration: (s) {
	// List of potential names for empty enum value
	const List<String> emptyEnumValues = ['empty', 'none', 'unknown'];
	// Ensure that the enum value is not empty
	value = emptyEnumValues.firstWhere(
	(e) => !s.values!.contains(e),
	orElse: () => '',
	);
	if (value.isEmpty) {
	throw Exception(
	"\n\nEmpty enum value found in schema '${schema.title}'\n",
	);
	}
	});
	}
	return value;
	}

[walsha2]

What is going on here, I understand that it's renaming the names of enum members, but what is this empty , unknown...?

In the event that there is an empty enum value, there is no real good solution for how to name it. Take the following valid example spec:

"StateLocation": {
  "type": "string",
  "enum": [
    "",
    "alabama",
    "alaska",
    "arizona",
    "arkansas",
    ...
  ],
}

The end user may be using "" to denote that the enum is not set. However, how do we name this enum in Dart code? The ['empty', 'none', 'unknown'] are an attempt to loop over those three candidate names, make sure none of them already exist in the enum, and use that as a placeholder name for the empty enum.

In summary: the spec did not provide a name, we need to come up with one. The reason there are three options is just to cover the case where, for example, "empty" is already an enum value in this enum - if so, try the next candidate name. This is not from a standard, just some convention I came up with to deal with this case.

[dickermoshe]

@walsha2
I'm using this PR to cleanup invalid property names.

E.G !@#$%^&* in am enum member or property name.
This means that if there is

• 1value
• 2value

it would remove that leading digit.
The issue is that we now have duplicate values

• value
• value

This will result in invalid code.
To handle this I will append a number to each duplicate.

• value1
• value2

[walsha2]

it would remove that leading digit.
The issue is that we now have duplicate values

Hmmm... Im not sure about this or why you are combining changes for a leading digit with changes to handle unescaped characters. These seem like two separate problems.

For the first one, enum value starts with a digit. There should already be protections for this. Are you somehow not triggering this part of the code in your test?

openapi_spec/lib/src/generators/schema.dart

Lines 965 to 968 in 8c239f9

	// Dart enums cannot start with a number
	if (value.startsWith(RegExp(r'[0-9]'))) {
	value = 'v$value';
	}

We should not be removing or moving values if we can help it. For example if the starting point is 1value I think the preferred output would be v1value as opposed to value1 (like you are showing above). This repo standardizes the use of a leading v to denote a value. In this example it looks a little funky, but take this enum:

"SizeEnum": {
  "type": "string",
  "enum": [
    "1x1",
    "2x2",
    "3x3",
    ...
  ],
}

The resulting Dart enums should/will be:

enum SizeEnum {
   v1x1,
   v2x2,
   v3x3,
}

I think this is preferable than your solution.

[dickermoshe]

@walsha2 Agree.
What about the following?

• !
• )

This needs to be removed, we then can end up with duplicates and many empty values

[dickermoshe]

For the first one, enum value starts with a digit. There should already be protections for this. Are you somehow not triggering this part of the code in your test?

Correct, I want to add it to parameters too.
Instead of copying this logic everywhere, it should be attached to the SchemaObject and SchemaEnum classes

[walsha2]

@walsha2 Agree. What about the following?

• !
• )

This needs to be removed, we then can end up with duplicates and many empty values

What is this real world use case exactly where someone is using this Dart generator on an enum value if ! and )? I am all for adhering to the OpenAPI spec, but at what cost. For these fringe case, I would rather forgo the additional complexity in the codebase until such time where someone requests this as a real need.

It seems like right now you are going through the OpenAPI examples and pulling out the most complicated/fringe cases to support in this repo, no? Do you actually have a use case that is being solved by adding this support to the generator? Just curious on the motivation for these code changes.

[dickermoshe]

The PR I'm adding will add a dartName and jsonName to every value in an enum and every property in a object.
Pushing later tonight, I think you'll like it.

I does it without changing much of the codebase. Streamlines parts of it.

[walsha2]

The PR I'm adding will add a dartName and jsonName to every value in an enum and every property in a object. Pushing later tonight, I think you'll like it.

Sounds good. I can take a look. I appreciate all the effort.

One thing that I think needs to happen before we incorporate any of these overhaul type changes is to first add a bunch of unit tests to ensure these cases are covered. Right now the unit tests are not nearly comprehensive enough and I also need to take a second and add some infra to compare the output code of each generated file against some archived, golden, version. This is on my to-do list here soon.

[dickermoshe]

Now

I agree this would be a big change.
However this PR is really minimal, it just makes the strings raw.
For the current PR and the other bugs, I'll wait for more tests

Goal

The user should never encounter a bug by themselves, the generator should warn them.
So even if we don't fix every random edge case, we should know what they are.

Proposal:
We add a function that checks the OpenApi if any of these issues will occur when generate runs.
This will give the user some idea on how to fix his schema to get the generator working.

var spec = OpenApi.fromString(
            source: rawSchemaText,
            format: OpenApiFormat.fromExtention(fileExtension))
        .centralizedSpec();

// New Function
checkSchemaAndWarn(spec ) 

await spec.generate( );

Expectations

A generator that handles every valid schema would be too complex, and keeping the source code neat it a worthy tradeoff.
You're correct about that for sure. I was being too ambitious.

Thanks!