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

Missing Query Schemas #108

Closed
simax opened this issue Feb 11, 2019 · 13 comments
Closed

Missing Query Schemas #108

simax opened this issue Feb 11, 2019 · 13 comments

Comments

@simax
Copy link

simax commented Feb 11, 2019

I recently reported in issue #92 that I now no longer produce an OAS in Swagger 2.0 format but now produce one in OpenApi 3.0.1 format. :

swagger.txt

Even though I seem to get better results with that, the results are still somewhat disappointing when using OASGraph to generate the schema and trying to use the generated OAS in GraphiQL.

The following problems still occur:

  1. SearchOrders (and possibly others) is missing from the schema and docs even though it's in the Swagger.json file.

  2. Also, as mentioned in issue #107 how can I get the docs to present the schema in alphabetical order?

@Alan-Cha
Copy link
Collaborator

Hi Simon! Thank you for your persistence in trying to make OASGraph work! Your feedback is so valuable to us as it helps us identify what we need to do to make OASGraph better. I hope you will continue to stick with us as we try to address these issues.

The missing operation is very strange. I briefly looked into it with @ErikWittern and I am about to take a deep dive into it right now.

I addressed #107 in the latest commit! Everything should be nice and alphabetized as soon as the pull request goes through.

@simax
Copy link
Author

simax commented Feb 13, 2019

Cool, thanks guys, the alphabetical order fix is working nicely!

@Alan-Cha
Copy link
Collaborator

I think I know what the problem is!

In GraphQL, field names must be unique. In the OAS you provided, there are a few operations that will receive the same field name from OASGraph. They are the following:

  • /api/v1/settings/plans/{id} and /api/v1/settings/plans/current, which create a plan field
  • /api/v1/orders and /api/v1/orders/search, which create an orders field
  • /api/v1/categoriesand /api/v1/categories/search, which create a categories field

I think it's bad that we don't throw a warning so I'll try to change that. Aside from that, it's hard to say what's the best way to move forward...

We could try to put some automatic mitigation in place... For example, we could append a number at the end of a duplicated field name. However, this can create other types of name collisions. I can go into more detail if you'd like.

Another method would be to define a new schema just so you can provide another name but this is a pretty ugly solution...

What do you think?

@ErikWittern
Copy link
Collaborator

@Alan-Cha In such cases, if present, how about relying on the operationId?

@Alan-Cha
Copy link
Collaborator

@ErikWittern It's a possibility but I think it's still vulnerable to name collisions. Another problem is that it makes naming inconsistent.

The query field names (as opposed to mutation field names) are based on the return type. The plan field returns a Plan object, the orders field returns an Orders object, and the categories field returns a Catergories object. Suddenly switching to the operationId may be jarring and is still open to the same problems as appending a number to the end of the field name.

Alan-Cha added a commit to Alan-Cha/oasgraph that referenced this issue Feb 13, 2019
Related to IBM#108

Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
Alan-Cha added a commit to Alan-Cha/oasgraph that referenced this issue Feb 13, 2019
Related to IBM#108

Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
Alan-Cha added a commit to Alan-Cha/oasgraph that referenced this issue Feb 13, 2019
Related to IBM#108

Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
ErikWittern pushed a commit that referenced this issue Feb 13, 2019
Related to #108

Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
@simax
Copy link
Author

simax commented Feb 15, 2019

@Alan-Cha I'm still a little confused. I'm sure you're right - it's to do with duplicates etc. In the attached swagger I've added some more endpoints and this time orders, orderSearch, plan, planCurrent and plans appear correctly. Categories also appears but CategorySearch does not. Does that still fit with your diagnosis? If so I'll wait for you guys to release the version with the duplicates warning and try again.

Incidentally, for me personally, OperationId would work well. I have full control over what the content of that is in the swagger.json file.

swagger .txt

@ErikWittern
Copy link
Collaborator

ErikWittern commented Feb 15, 2019

@simax I think that is all still in line with the diagnosis. We specifically tried to use the name of the schema for fields, i.e., if a GET operation defines a return schema:

content: 
  application/json:
    schema:
      $ref: '#/components/schemas/Orders'

...OASGraph will try to call the field orders (based on the Schema name). The problem now is that multiple operations return data following the Orders schema.

By the way, we made an explicit decision for using schema names, as we observed that many OAS contain operation ids like getUsers or similar, which would lead to an "RPC-style" look of the GraphQL interface. We will have to come up with a way to avoid collisions, though.

One idea @Alan-Cha proposed was to provide an option for users to chose to use operationIds to name fields (in the root object). This would give developers full control (if desired), but leave schemas looking nice per default (and throw warnings if there are conflicts).

@Alan-Cha
Copy link
Collaborator

@simax Yes, the diagnosis is still true. It is weird that orders and orderSearch, for example, appears correctly in the updated schema but I can explain that.

OASGraph already has some soft query field name collision mitigation based on the operationId. OASGraph will try to use the type name to create a field name. However, if the field name is already taken, it will try to use the operationId.


The problem with the current mitigation is that the order in which OASGraph processes these operations is not always clear. After all, it is simply going through the unsorted properties of a JSON object (the OAS) one by one.

To demonstrate, let's look at GET /api/v1/orders/search, which has the operationId: SearchOrders, and GET /api/v1/orders, which has the operationId: Orders. Both operations will return an Order type.

I believe when OASGraph processes the first schema, it processes the GET /api/v1/orders/search first. Because no order field has been defined, it will get the order field name. OASGraph then processes GET /api/v1/orders. Because the order field has already been defined, OASGraph will derive a new field name from the operationId Order. Unfortunately, the adapted field name is still order and therefore, it will overwrite the previous field.

When OASGraph processes the second schema, it processes GET /api/v1/orders first. GET /api/v1/orders will get the order field name, and later, GET /api/v1/orders/search will get the searchOrders field name, adapted from the SearchOrders operationId.

In the first case, an operation is missing but in the second, both operations exist as expected.


I think a hard solution to this problem is to create an option that allows you to manually define field names from operationIds, as @ErikWittern mentioned. I will try to get this out hopefully by the end of the day!

In regards to automatic mitigation, I think there is still room for improvement.

Alan-Cha added a commit to Alan-Cha/oasgraph that referenced this issue Feb 15, 2019
Related to IBM#108

Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
@simax
Copy link
Author

simax commented Feb 18, 2019

Great, thanks for the explanation I'll look out for your next release.

Alan-Cha added a commit to Alan-Cha/oasgraph that referenced this issue Feb 19, 2019
Related to IBM#108

Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
ErikWittern pushed a commit that referenced this issue Feb 19, 2019
Related to #108

Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
@Alan-Cha
Copy link
Collaborator

@simax We just released OASGraph v0.14.0 which can support all operations in both of the OASs you provided using the new operationIdFieldNames feature. Let us know how it goes for you!

@simax
Copy link
Author

simax commented Feb 21, 2019

Awesome, thanks guys. I just tried this out and it's working well. Much appreciated!

@simax simax closed this as completed Feb 21, 2019
@lhr0909
Copy link

lhr0909 commented Feb 23, 2019

Thanks @Alan-Cha ! We did a manual patch a few months back to make it through a project, and always wanted to report and help add an option like this! Glad this is just resolved and we don't need to use our fork anymore.

@Alan-Cha
Copy link
Collaborator

@lhr0909 Thanks for letting us know! Always feel free to report new issues and help contribute to OASGraph. We love hearing from our users, even if it's not always positive news 😆 Glad to hear other people think this option is useful too!

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

No branches or pull requests

4 participants