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

added clarification on namespaces, characters in definitions name #634

Merged
merged 3 commits into from
Apr 14, 2016

Conversation

fehguy
Copy link
Contributor

@fehguy fehguy commented Apr 8, 2016

Fixes #578

@IvanGoncharov
Copy link
Contributor

@fehguy It creates a problem with discriminator since definitionsName used as a value of discriminator.

If I use some non-ASCII letters in discriminator value for example letters from non-English alphabets.
I think this change is superb and should be merged but discriminator should be redesigned to not use definitionsName

<a name="definitionsName"></a>{name} | [Schema Object](#schemaObject) | A single definition, mapping a "name" to the schema it defines.
<a name="definitionsName"></a>{name} | [Schema Object](#schemaObject) | A single definition, mapping a "name" to the schema it defines. The following characters are legal in the `definitionsName`: `a-zA-Z0-9.-_()[]\/`

The following characters are illegal in the `definitionsName`: `"'`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spaces are also not allowed in this proposal

@webron
Copy link
Member

webron commented Apr 11, 2016

Somewhat indifferent to this but learning towards no-merge. Given the allowed characters, there are still going to be cases where preprocessing and escaping may be required by some tools and languages, so other than adding a seemingly arbitrary list of characters.

If others at the @OAI/tdc prefer to see it go on, I'm okay with it.

@fehguy
Copy link
Contributor Author

fehguy commented Apr 11, 2016

@webron you'd prefer the legal characters for schema names to be undefined? So we can have this name as a model? #/definitions/myModel?

@fehguy
Copy link
Contributor Author

fehguy commented Apr 11, 2016

@OAI/tdc please approve or reject

@whitlockjc
Copy link
Member

Could you squash your commits upon merge? Other than that, LGTM.

@darrelmiller
Copy link
Member

Unless someone can suggest a less arbitrary set of characters to allow, I'm 👍 on this. They seem like a reasonably decent of characters to allow in identifiers.

@webron
Copy link
Member

webron commented Apr 12, 2016

@fehguy - the lack of character restriction hasn't been devastating until now. However, I agree that with a fixed set of characters, escaping should be easier where needed. So, 👍 .

@@ -1945,7 +1945,7 @@ An object to hold data types that can be consumed and produced by operations. Th

Field Pattern | Type | Description
---|:---:|---
<a name="definitionsName"></a>{name} | [Schema Object](#schemaObject) | A single definition, mapping a "name" to the schema it defines.
<a name="definitionsName"></a>{name} | [Schema Object](#schemaObject) | A single definition, mapping a "name" to the schema it defines. The following characters are legal in the `definitionsName`: `a-zA-Z0-9.-_()[]\`. All other characters, including high-level characters, are not legal in the `definitionsName`. The name may be used to namespace the definitions--it is the tooling vendor's job to ensure that the namespace is honored.
Copy link
Contributor

Choose a reason for hiding this comment

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

@fehguy Just a few thoughts:

  1. "high level characters" could be potentially misinterpreted. For example, if I google "high level characters", I get this https://en.wikipedia.org/wiki/Epic_level. Perhaps "special characters", which is in more common use (and what I think you meant).
  2. An example of what a namespace might look like would be useful here for someone trying to understand what namespace means in this context.

Copy link
Member

Choose a reason for hiding this comment

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

I like both @jasonh-n-austin 's suggestions. Myself, I hesitate about including ()[]\, since the characters .-_ give a fair bit of punctuation to work with already.

@earth2marsh
Copy link
Member

@jasonh-n-austin 's suggestions would improve this somewhat, but even so, 👍

@fehguy
Copy link
Contributor Author

fehguy commented Apr 14, 2016

I've simplified the allowable characters and added examples.

@fehguy fehguy merged commit 86b99d3 into OpenAPI.next Apr 14, 2016
@netmilk
Copy link

netmilk commented Apr 14, 2016

@fehguy @webron I know you snooze you loose, but is there any way how to revert this? I think it's a slightly unfortunate limitation. This limitation for type/schema/definitions names isn't present in any other API description languages and it can kill the seamless interoperability and lossless conversion between formats. It can mainly hurt migration to OAS from other formats, not the other way round.

@ralfhandl
Copy link
Contributor

@fehguy this is a serious restriction. Our definition names can consist of everything allowed in JavaScript variable names plus the dot as a namespace separator. Having to map that into a-zA-Z0-9.-_\ would make things really awkward.

Please revert.

@fehguy
Copy link
Contributor Author

fehguy commented Apr 14, 2016

@netmilk @ralfhandl please give some suggestions. Leaving it open-ended has created more problems that I care to go into without a drink in hand. Some restrictions need to apply, so how about some examples?

Do you really want a model called "#/definitions/myModel"? Wouldn't that be a disaster too? So there's a middle ground between "allowing everything" and "[a-zA-Z]" that should be struck.

@fehguy
Copy link
Contributor Author

fehguy commented Apr 14, 2016

@netmilk I really don't think that the freedom in other formats should be the driving factor for the OAI. But as requested, please provide some examples, I do think the restrictions proposed cover most reasonable situations, and avoid you naming a schema something like ☺.

@ralfhandl
Copy link
Contributor

@fehguy My suggestion for a middle ground is JavaScript Identifier characters plus the dot. This is not unusual in namespace names of programming languages.

I recently started to use parentheses in addition because definition names show up prominently in Swagger UI and the artificial names Swagger UI invents for anonymous nested types were perceived as confusing by developers. So I introduced explicit names for collection types, e.g. Collection(Meine.Übersetzung).

@fehguy
Copy link
Contributor Author

fehguy commented Apr 14, 2016

@ralfhandl I understand very well that any change in the spec will cause changes in tooling, Swagger-UI being one of them. Using the existing Swagger-UI as a reason to not change the spec is quite backwards. We will change Swagger-UI to follow the spec (not vice versa).

For your example, you should actually be using a collection (array) model, with an inner type.

@darrelmiller
Copy link
Member

darrelmiller commented Apr 14, 2016

@ralfhandl The regex for validating JavaScript variable names is 11,236 characters long I'm not sure we want to deal with that.

@ralfhandl
Copy link
Contributor

@fehguy apparently I didn't make my concern clear enough. A natural translation of OData types into OAS is turning each OData type into a Definitions Object. The natural name for that Definitions Object is the OData type name, e.g. Meine.Übersetzung.

If OAS reduces the set of characters for definition names as suggested this is no longer possible. So an OData-to-OAS transformation will have to invent unnatural names that conform to the OAS restrictions and differ from the OData names. This could be by encoding missing characters, or by using random numbers as OAS definition names. Both would reduce readability of the OAS files.

@darrelmiller
Copy link
Member

@ralfhandl Don't those same OData types have to eventually get mapped into types in programming languages like C# and Java? I'm pretty sure C# doesn't allow most delimiter characters in identifier names. I don't know about Java.
If your concern is about supporting non-english characters such as those with accents, etc, then I think that is worth discussing.

@fehguy
Copy link
Contributor Author

fehguy commented Apr 14, 2016

FYI Java says "no" to non ASCII characters in source code and as file/class names (among other things).

@darrelmiller
Copy link
Member

Unfortunately the C# spec's rules on identifiers is pretty much unparsable for Unicode neophytes like myself.

@ralfhandl
Copy link
Contributor

ralfhandl commented Apr 14, 2016

The OData identifier naming rules actually follow C#. OData SimpleIdentifiers are valid C# and JavaScript variable names, and OData namespace identifiers are valid C# namespace identifiers.

Trying to understand the naming rules for both C# and JavaScript identifiers taught me a lot about Unicode :-)

A learning effort that so far was unnecessary for OAS fans as there were no naming rules for Definitions Object names. Some things were better in the past ;-)

@ePaul
Copy link
Contributor

ePaul commented Apr 14, 2016

FYI Java says "no" to non ASCII characters in source code and as file/class names (among other things).

Huh? In Java you can use about any Unicode letter (and also some other characters like numbers) in identifiers. (Though it might be unwise to use those for file names – and thus for names of classes –, due to encoding problems in different operating systems.)

@fehguy
Copy link
Contributor Author

fehguy commented Apr 14, 2016

Guys, here's my position. The spec is language independent. It cannot and should not be forced into a single language syntax. Just as Java doesn't allow a "." in the classname, translation from parens, etc., just isn't practical for every situation.

I do believe that the proposed set of characters is quite adequate for translation to/from other languages without being bound to a single one. It addresses the complex need for namespacing (allowing the ".") and keeps us from causing serious issues with code generators by supporting non ascii characters.

@ralfhandl for the use case you're describing, I do believe that a x-odata-identifier is exactly what is needed for supporting a specific structure that can be translated to/from your language.

@darrelmiller
Copy link
Member

Ok, so from the OData ABNF doc, I found this,

odataIdentifier = identifierLeadingCharacter *127identifierCharacter
identifierLeadingCharacter = ALPHA / "" ; plus Unicode characters from the categories L or Nl
identifierCharacter = ALPHA / "
" / DIGIT ; plus Unicode characters from the categories L, Nl, Nd, Mn, Mc, Pc, or Cf

I'm not sure where to find out what those Unicode characters are.

@ePaul
Copy link
Contributor

ePaul commented Apr 14, 2016

I'm not sure where to find out what those Unicode characters are.

https://en.wikipedia.org/wiki/Unicode_character_property#General_Category
http://www.fileformat.info/info/unicode/category/index.htm

I guess for actually using this you'll need a unicode character database build into your tool (or your programming language/framework). The point is that by referring to Unicode you don't have to do this decision yourself in your spec, and someone else takes care of updating it for you.

@fehguy
Copy link
Contributor Author

fehguy commented Apr 14, 2016

@ePaul oops, you're right. I think it was burned in my head so strongly that "non ascii characters in public java classes are grounds for dismissal" that I thought of it as a real rule, beyond one to just "keep my job".

@ralfhandl
Copy link
Contributor

@fehguy Apparently OData isn't the only "language" with this problem, Java, JavaScript, C#, and Swift all allow Unicode letters in identifiers. So why not add a first-class identifier to OAS if you really want to restrict Definitions names to a subset of programming language identifiers? This would allow developer-facing tools like Swagger UI to use this identifiervalue when visualizing the API structure.

@earth2marsh
Copy link
Member

In an ideal world, I'd want to empower people to do what worked for them. But the tradeoff is complexity burden on tooling folks, who will have to deal with debugging languages that are unfamiliar to them. So much of the spec is already biased toward English (all the key names), so I'm ok extending that logic for model names. Keeping the testing/maintenance/support burden lower on tooling authors should be a goal of the project.

The other issue here is what punctuation should be allowed. IMO, the benefits of allowing things like brackets in a name has not be articulated enough as to make it an obvious candidate, therefore I prefer the restricted characters.

@ralfhandl
Copy link
Contributor

ralfhandl commented Apr 18, 2016

What exactly is the benefit/simplification for tooling authors?

And how does this offset the drawback/complication for tool users that now have to encode existing model names so that they fit into the reduced character set?

In an ideal world there are typically more developers using tools around a given spec than developers producing tools for that spec.

@ralfhandl
Copy link
Contributor

How to resolve the issue with discriminator that @IvanGoncharov pointed out?

APIs using discriminator with the flexible model names allowed in spec 2.0 would be forced to produce different discriminator values in responses. Thus switching to a different version of the API description language would require publishing new, incompatible versions of the described APIs.

Is this desirable?

@IvanGoncharov
Copy link
Contributor

@ralfhandl people already unhappy with discriminator values as module names.
I will open new issue to address it, by making discriminator directly map values to $refs.

IMHO, OpenAPI spec is pretty complicated so you can't make changes without some other parts of it.
Important thing it should be addressed before public release of OpenAPI v3.0

@earth2marsh
Copy link
Member

Until we declare a reason to change this, the current approach should stand.

@fehguy fehguy deleted the issue-578 branch October 20, 2016 17:12
handrews added a commit to handrews/OpenAPI-Specification that referenced this pull request Oct 11, 2024
added clarification on namespaces, characters in definitions name

[Manually ported merge]

Co-authored-by: Tony Tam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants