[REQ] Define custom reserved words

[lukepighetti]

There are some words that should be reserved in my project using dart-dio-next that are not. Namely Type type, and toString field, along with update field that conflicts with BuiltValue classes

It would be nice if we could pass our own reserved words as needed

[0xNF]

@kuhnroyal

This seems loosely related to the PR that was accepted last month fixing #12127. There's a lot of implicitly reserved keywords that the Dart generator is using that aren't conveyed to the end users easily.

I thought about going through and finding all the places that local variables are declared and replacing them _$x, as in Type => _Type, but the other idea I had was to maybe hjiack a feature that already exists:

\modules\openapi-generator\src\main\resources\dart\dart-keywords.txt contains a list of keywords which, if encountered by the generator during parsing of the OpenAPI specification file, will automatically append the conflicting keyword with an _.

I think that this list is supposed to be a reflection of what the official Dart reserved keywords are, but if we add in all the local variables the generator declares for itself in the various implementations of the toJson, fromJson, and other methods that directly deal with user-defined fields, then we can safely permit the user to use any word they desire in their specification files.

What do you think?

[kuhnroyal]

Yea that could work, not sure what we currently check against that list. types/properties, not sure about parameters.
Also not sure if there is an existing generator option for this.

[lukepighetti]

The problem with leading underscores is that in Dart that makes a member private

Unlike Java, Dart doesn’t have the keywords public, protected, and private. If an identifier starts with an underscore (_), it’s private to its library.

https://dart.dev/guides/language/language-tour#libraries-and-visibility

[kuhnroyal]

@lukepighetti https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractDartCodegen.java#L315

[0xNF]

Yes you're right. Changes to user variables would, and do in the cases where it currently works, get a trailing underscore.

I was talking about leading underscores for actually private variables reserved by the generator itself such as the castMapOfType method names, or the queryParameters variable names, both of which are internal generator names.

In any event, I still prefer adding those items to the keywords list instead, since I think it will automatically solve the issue.

[kuhnroyal]

I would not automatically add anything, because we will not be able to keep it in sync for all dart generators anyways.
There is a --reserved-words-mappings option which may work out of the box.
If the generator does not support this, or if we overwrite the default, then we need to fix this.

[0xNF]

we will not be able to keep it in sync for all dart generators

Why not? I don't see why it'd be very hard for the Dart portion maintainers to keep a list of all the variables introduced by the .mustache files, and then drop them into the keywords.txt if the method they're a part of touches user generated variables.

As an example, I added the following words to the dart-keywords.txt file:

apiClient
parameterToString
mapValueOfType
mapCastOfType
mapDateTime
json
requiredKeys
path
postBody
queryParams
headerParams
formParams
contentTypes
mp
type
hasFields
responses
responseBody

I used the following spec file:

openapi: 3.0.3
info:
  version: "1.1"
  title: Dart Reserved Words
servers:
  - url: 'localhost'
    variables:
      host:
        default: localhost
components:
  schemas:
    ItemWithReservedKeywords:
      type: object
      properties:
        switch:
          type: int
        apiClient:
          type: int
        parameterToString:
          type: int
        mapValueOfType:
          type: int
        mapCastOfType:
          type: int
        mapDateTime:
          type: int
        json:
          type: int
        requiredKeys:
          type: int
        path:
          type: int
        postBody:
          type: int
        queryParams:
          type: int
        headerParams:
          type: int
        formParams:
          type: int
        contentTypes:
          type: int
        mp:
          type: int
        type:
          type: int
        hasFields:
          type: int
        responses:
          type: int
        responseBody:
          type: int
paths:
  /items:
    get:
      operationId: GetItemWithMapStringObjects
      responses:
        "200":
          description: get the item
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ItemWithReservedKeywords'
    post:
      operationId: UpdateItemWithMapStringObjects
      description: Updates the item
      parameters:
        - in: query
          required: true
          name: path
          description: path
          schema:
            type: int
        - in: query
          required: true
          name: postBody
          description: path
          schema:
            type: int
        - in: query
          required: true
          name: contentTypes
          description: path
          schema:
            type: int
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/ItemWithReservedKeywords'
      responses:
        "200":
          description: "success"
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ItemWithReservedKeywords'

And then the generated output was this (just the fromJson method for brevity):

  static ItemWithReservedKeywords? fromJson(dynamic value) {
    if (value is Map) {
      final json = value.cast<String, dynamic>();

      // Ensure that the map contains the required keys.
      // Note 1: the values aren't checked for validity beyond being non-null.
      // Note 2: this code is stripped in release mode!
      assert(() {
        requiredKeys.forEach((key) {
          assert(json.containsKey(key), 'Required key "ItemWithReservedKeywords[$key]" is missing from JSON.');
          assert(json[key] != null, 'Required key "ItemWithReservedKeywords[$key]" has a null value in JSON.');
        });
        return true;
      }());

      return ItemWithReservedKeywords(
        switch_: mapValueOfType<int>(json, r'switch'),
        apiClient_: mapValueOfType<int>(json, r'apiClient'),
        parameterToString_: mapValueOfType<int>(json, r'parameterToString'),
        mapValueOfType_: mapValueOfType<int>(json, r'mapValueOfType'),
        mapCastOfType_: mapValueOfType<int>(json, r'mapCastOfType'),
        mapDateTime_: mapValueOfType<int>(json, r'mapDateTime'),
        json_: mapValueOfType<int>(json, r'json'),
        requiredKeys_: mapValueOfType<int>(json, r'requiredKeys'),
        path_: mapValueOfType<int>(json, r'path'),
        postBody_: mapValueOfType<int>(json, r'postBody'),
        queryParams_: mapValueOfType<int>(json, r'queryParams'),
        headerParams_: mapValueOfType<int>(json, r'headerParams'),
        formParams_: mapValueOfType<int>(json, r'formParams'),
        contentTypes_: mapValueOfType<int>(json, r'contentTypes'),
        mp_: mapValueOfType<int>(json, r'mp'),
        type_: mapValueOfType<int>(json, r'type'),
        hasFields_: mapValueOfType<int>(json, r'hasFields'),
        responses_: mapValueOfType<int>(json, r'responses'),
        responseBody_: mapValueOfType<int>(json, r'responseBody'),
      );
    }
    return null;
  }

So it seems like this approach works pretty well.

[kuhnroyal]

Why not?

That's just an observation on large projects with different, changing contributors. Someone will forget.

This is also a corner case for most users and there is an existing generator option for this case which additionally allows to set the replacement.

I'll have a draft PR up shortly.

[0xNF]

The problem I see is how to let users know that their chosen keywords are going to blow up on them before they do?

I only found out at runtime for the json variable name a while ago, for instance. Is it permissible to let users find out so late in the process about internal keywords that just happen to be owned by the generator?

[kuhnroyal]

Yea I think so, because that is also the cases with conflicting type names which can be changed via the type mapping.
For some of the variables like json we can should try to find some better variables with some smart pre/suffixes.

[0xNF]

It seems like it's just kicking the can further down the road, and further into the users lap though.

Changing json to json_ and queryParams to queryParams_ is good, but without marking those as reserved, now we've just made life hard for the poor sap who has those in their schema. How many of those people exist out in the world, I don't know. But it shouldn't be their responsibility to know what the project reserves for itself.

If the group maintaining their portion of the project think that keeping the reserved variable list up to date is too much work... I don't know, that doesn't sit well with me. It really shouldn't be the users responsibility to divine what's owned and what isn't.

I think an ideal resolution is to pick variable names with a low chance of collision (underscores where appropriate), and also to add those remaining keywords to the reserved keyword list, because that's why they are.

The switch you've uncovered is a great escape hatch for when things that are forgotten by the maintainers are encountered by the user. But we know about these items in advance. Leaving this to the user seems negligent and hostile.

[kuhnroyal]

This is not done for any generator in this project, as far as I know.

[0xNF]

I see. Then you're right that this shouldn't be up to the dart portion of the project to solve on its own. It sounds like a more general issue that should be handled by the wider project.

[lukepighetti]

I don't have much to add here except that it's unreasonable to ask publishers to change their schema to be compatible with different languages. We should be able to take any valid schema and generate a valid client in any language.

If there aren't e2e tests for generation, perhaps there should be. Some ideas: Twitter, Coda, Jira against Swift, Go, Kotlin, Dart. If that matrix cannot be generated with no static analysis errors, there's probably an issue in openapi-generator