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

Non-unique operationId #137

Closed
jayvdb opened this issue Jul 31, 2020 · 15 comments
Closed

Non-unique operationId #137

jayvdb opened this issue Jul 31, 2020 · 15 comments
Labels
question Further information is requested

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Jul 31, 2020

operationId | string | Unique string used to identify the operation. The id MUST be unique among all operations described in the API. The operationId value is case-sensitive. Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is RECOMMENDED to follow common programming naming conventions.

  /oscarapi/checkout/payment-states/:
    get:
      operationId: oscarapi_checkout_payment_states_retrieve
      ...
  /oscarapi/checkout/payment-states/{id}/:
    get:
      operationId: oscarapi_checkout_payment_states_retrieve
      ...
@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 31, 2020

fwiw, my frontend code generator https://github.com/cyclosproject/ng-openapi-gen renames the second one oscarapiCheckoutPaymentStatesRetrieve_1.

@tfranzel
Copy link
Owner

tfranzel commented Aug 6, 2020

hey @jayvdb sry for the delay. a lot of stuff going on right now.

to me it looks like it should be _list and *_retrieve for {id}. i'm assuming the is_list_view heuristic is not detecting that case properly. we introduced @extend_schema_serializer(many=True/False) to override the detection for those rare cases. does that work for you?

@tfranzel tfranzel added the question Further information is requested label Sep 7, 2020
@jayvdb
Copy link
Contributor Author

jayvdb commented Sep 7, 2020

It isnt a rare case. I have hundreds of them. Almost every use of {format} creates a dup.

@tfranzel
Copy link
Owner

tfranzel commented Sep 7, 2020

ok so lets take this apart real quick to not confuse the issues, because i see two separate ones here.

  • issue 1 (your initial example): retrieve .../ and list .../{id} both get recognized as _retrieve even though one must be _list. that is a shortcoming of the listing detection heuristic, which is pretty much at the limit. not sure if that can be improved further without breaking other things.
  • issue 2 {format}: having two operations (one with {format} and one without) that have the same operation_id.

i meant that issue 1 is "usually" rare (but maybe not for oscar). of course issue 2 may be very common.
i believe those two are more or less orthogonal. which one are you referring to here? issue 2 should be easily fixable.

@jayvdb
Copy link
Contributor Author

jayvdb commented Sep 7, 2020

This issue is about any and all dup operation id spec violations caused by path params. I'm not interested in discussion about splitting it into subsets. The cause is path params. Lots of apps have them.

@tfranzel
Copy link
Owner

tfranzel commented Sep 7, 2020

hmm, i get that the outcome is the same (operationId collision), but i have a hard time reasoning about possible fixes without having a look at the cases in detail. mainly because there is likely no magic bullet, at least it is not apparent to me at the moment. i think there are 3 types:

1: DRF's magic {id}: list/retrieve collision on failing heuristic (difficulty: hard)
2: DRF's magic {format}: format variant not reflected in operationId (difficulty: easy)
3: user defined path parameter: there are a lot of possibilities here. (difficulty: unknown)

one could easily fix all 3 by simply encoding ALL path params into the operationId name, which will be quite ugly i suppose. Pretty sure that is not a good solution (e.g. oscarapi_checkout_payment_states_ID_FOOPARAM_BARPARAM_retrieve). do you have any ideas or suggestions?

@jayvdb
Copy link
Contributor Author

jayvdb commented Sep 7, 2020

Including path params to operation Id seems sane to me.

They end up as method names in generated code, so it would be nice to keep them short and meaningful, and they must be stable otherwise changes will cause non-stable generated code.

I refuse to have hashes in source code symbol names , so I do hope that isnt repeated , but I already have my own solution so I ultimately dont care what approach is taken.

ng-openapi-gen postfixes the operation Id with a sequence number for duplicates, which is fairly stable, if only because the order of the route endpoints can be finely controlled if necessary using DEFAULT_GENERATOR_CLASS (esp. by a custom the ENDPOINT_ENUMERATOR_CLASS).

@tfranzel
Copy link
Owner

tfranzel commented Sep 9, 2020

ok i understand.

I refuse to have hashes in source code symbol names , so I do hope that isnt repeated

i was not aware of this. how and where do hashes get into the names? i would like to fix that.

i did some improvement to format names. not a big fan of having all variables in the operation_id (except for formatted), because it will make the generated code names weird imho. collision resolution i think we can do. i'll try to come up with something there.

@jayvdb
Copy link
Contributor Author

jayvdb commented Sep 9, 2020

where do hashes get into the names?

enum class names.

@tfranzel
Copy link
Owner

tfranzel commented Sep 9, 2020

not sure how that can happen. in the operationId? is this a oscar thing? can you point me in the right direction there?

just to make sure, you are not referring to the enum postprocessing hook. the names there are derived from either field name or manual name override. a field name with a # in it should not be possible.

@tfranzel
Copy link
Owner

  • implemented collision resolution for operationIds (e.g. oscarapiCheckoutPaymentStatesRetrieve1 with camelize and _1 without)
  • could not reproduce the initial issue with and without {id} both named retrieve. my oscar API example setup never exhibited that behavior, because all included views properly used RetrieveModelMixin and ListModelMixin.
  • for me there is only one collision for oscar-api, which is related to breadcrumbs and a weird re-usage of views, but that get resolved now by the collision detection. we can revisit optional path var inclusion into operationId in a dedicated issue, but i currently deem it low prio with collision resolution in place and it happening only once for standard oscar-api.
  • could not reproduce hashes in "enum class names". please open a dedicated issue for that.

@quarantin
Copy link

quarantin commented Jun 20, 2023

Not sure if this is the exact same problem, but I encountered this warning:

:warn(f'operationId "{operation_id}" has collisions {paths}. resolving with numeral suffixes.')

Turns out I had two endpoints like this:

  • /survey/
  • /survey/{slug}/

Each with a POST handler in different APIViews.

I simply fixed it using this decorator in one of the views:

+from drf_spectacular.utils import extend_schema

def class SurveyAPIView(APIView):
    ...
+   @extend_schema(operation_id="some_unique_name_create")
    def post(self, request, *args, **kwargs):
        ...

Not sure if this is the proper way to address this issue but my schema is OK and the warning is gone.

I hope this will help someone, I know it would've helped me.

@mattcousins
Copy link

I have been experiencing the issue with xy_retrieve and xy_retrieve_2 when it should be a list and a retrieve. I have narrowed it down to accessing self.request or self.instance in the init of a ModelSerializer child class. Print out self.request in the init and it happens, remove the print and it doesn't happen. Maybe this will help to fix this issue. Thanks

@tfranzel
Copy link
Owner

tfranzel commented Mar 5, 2024

@mattcousins that is certainly not the reason. The name resolution is not perfect. In the the rare case that a collision happens, it is recommended to resolve the issue via decorating one method with @extend_schema(operation_id=....

List detection is slightly different. Decorate @exend_schema(responses=YourSerializer(many=True)) and the second retrieve should now be a list. Not exactly sure how that happens for you though. The original issue is your list/retrieve situation.

@mattcousins
Copy link

Thanks for your reply, I understand that and can workaround. I was just saying I could reliably repeat a naming resolution issue so thought I would share with you in case it was helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants