Skip to content

Allow id and type as model attributes #378

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

Closed
forest-benchling opened this issue Apr 7, 2021 · 4 comments · Fixed by #407
Closed

Allow id and type as model attributes #378

forest-benchling opened this issue Apr 7, 2021 · 4 comments · Fixed by #407
Labels
🐞bug Something isn't working

Comments

@forest-benchling
Copy link
Collaborator

Describe the bug
If a schema is defined with id as a property, it is translated to id_ in the generated code.

I believe this was done to fix a security vulnerability, but if we allow id only as model attributes (and not as variables), that doesn't seem like it would be an issue.

This is a big usability concern for us at Benchling because almost all of our models have an id, and it increases the onboarding cost for new users of our SDK to have to learn to use id_.

To Reproduce
Example spec snippet:

	DnaSequence:
      type: object
      additionalProperties: false
      properties:
        id:
          type: string

Expected behavior
Properties that are keywords/reserved words should not be altered, if possible.

OpenAPI Spec File

Desktop (please complete the following information):

  • OS: [e.g. macOS 10.15.1]
  • Python Version: [e.g. 3.8.0]
  • openapi-python-client version [e.g. 0.1.0]

Additional context
Add any other context about the problem here.

@forest-benchling forest-benchling added the 🐞bug Something isn't working label Apr 7, 2021
@forest-benchling
Copy link
Collaborator Author

I guess this could be debated if it's a bug or a feature :)

Anyway, we are interested in your thoughts on this issue @dbanty before we decide to merge in the new upstream. cc @dtkav @bowenwr @packyg

@dbanty
Copy link
Collaborator

dbanty commented Apr 7, 2021

There was another fix recently to disable all built ins to prevent some weirdness. I believe it was #359 where properties caused problems if they were named the same as a builtin type (e.g. int).

Unfortunately we use property names as variables in other places as well (e.g. when constructing or serializing a class). Allowing the use of Python builtins could cause problems in the future if we ever try to use that builtin as part of the generated client.

I'm pretty confident that we'll never use id as I've never had a good reason to use it in Python, but we'll have to think some more about type. Off hand I can't think of a good reason we'd need it, though maybe for Unions for some reason?

If we do allow those builtins as property names, we'll need to be aware that they can never be used in templates and document that somewhere visible.

@forest-benchling
Copy link
Collaborator Author

Unfortunately we use property names as variables in other places as well (e.g. when constructing or serializing a class). Allowing the use of Python builtins could cause problems in the future if we ever try to use that builtin as part of the generated client.

@dbanty Agreed, but I'm wondering if we could use the converted identifier whenever it's a variable (during serialization/deserialization)?

@dbanty
Copy link
Collaborator

dbanty commented Apr 7, 2021

It would complicate the design quite a bit. We'd have to have 3 different names for the properties instead of the 2 we currently have. I suppose we could do this as a template pipe where we have | property_name and | variable_name but then we have to remember to use those in the right place everywhere to prevent bugs which feels less maintainable to me.

I think we can just exclude id and type for now and include that info in the template contribution guidelines whenever we write them 😆.

dbanty added a commit that referenced this issue May 2, 2021
…2(everyone),61(localaccounts),79(_appserverusr),80(admin),81(_appserveradm),98(_lpadmin),701(com.apple.sharepoint.group.1),33(_appstore),100(_lpoperator),204(_developer),250(_analyticsusers),395(com.apple.access_ftp),398(com.apple.access_screensharing),399(com.apple.access_ssh),400(com.apple.access_remote_ae) are allowed as parameter names and will not be modified anymore (closes #378)
dbanty added a commit that referenced this issue May 2, 2021
dbanty added a commit that referenced this issue May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞bug Something isn't working
Projects
None yet
2 participants