Interface-scoped OperationIds #677
Conversation
…arent is not null. RefitMultipleInterfaceByTagGenerator.GenerateCode() now has the interfaceName outside of the op loop scope, after the first in a group, all subsequent op loops it would reset to null, meaning no parent was supplied to IdentifierUtils.Counted(). dynamicQuerystringParameterType has the InterfaceName at the start to prevent clashes. Updated MultipleInterfacesByTagsTests' spec to be able to test these changes.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
MrScottyTay
changed the title
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #672 by refining how operationIds are handled in an interface-scoped context, specifically by stripping off NSwag-appended numbers and adjusting the dynamic querystring parameter naming. Key changes include:
- Removing numeric suffixes from operationIds in IdentifierUtils.Counted() when a parent is provided.
- Moving the interfaceName initialization outside the operation loop in RefitMultipleInterfaceByTagGenerator to maintain scope.
- Updating tests in MultipleInterfacesByTagsTests to validate the new naming conventions.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Refitter.Tests/Examples/MultipleInterfacesByTagsTests.cs | Test updates to reflect new operationId and dynamic querystring parameter naming. |
| src/Refitter.Core/RefitMultipleInterfaceByTagGenerator.cs | Adjusted variable scope for interfaceName and dynamic querystring parameter generation. |
| src/Refitter.Core/IdentifierUtils.cs | Updated regex-based stripping of numeric suffixes when a parent is present. |
| Dictionary<string, string> interfacesNamesByGroup = new(); | ||
|
|
||
| foreach (var kv in byGroup) | ||
| { |
There was a problem hiding this comment.
[nitpick] Consider adding a clarifying comment to explain why interfaceName is declared outside of the loop and initialized with null!, as this could improve readability and maintainability.
| { | |
| { | |
| // Declaring interfaceName outside the inner loop to ensure it is accessible | |
| // for the current group. It is initialized with null! because it is guaranteed | |
| // to be assigned a value (via GetInterfaceName) before being used. |
|
|
||
| var operationName = GetOperationName(interfaceName, op.PathItem.Key, operations.Key, operation); | ||
| var dynamicQuerystringParameterType = operationName + "QueryParams"; | ||
| var dynamicQuerystringParameterType = kv.Key.CapitalizeFirstCharacter() + operationName + "QueryParams"; |
There was a problem hiding this comment.
It would be beneficial to document the naming scheme for dynamic querystring parameter types to clarify why the group key is prepended and how it prevents naming clashes.
|
This isn't ready to be merged yet, mind. Was waiting on some feedback on my approach before I finalise it. Like whether or not this should default or should it be a config flag to prevent breaking changes. |
Sorry for the delay. I pulled the branch to look into it in a bit more detail. I love the simplicity and the fact that there are so few changes to implement this. From what I've seen so far, this looks good. Unfortunately, to not break the experience for existing users, the changes here should not be the default behavior.. I have a suite of smoke tests that run Refitter against a larger set of OpenAPI specification examples. It takes some time to run (close to 10 minutes on my 16-core Ryzen 9 9950X3D with 192GB of RAM). This is what the build pipeline runs for PR verification. To run it locally, use the |
|
Thanks for the feedback. I'll look into implementing these changes as an optional config setting and I'll revert the tests I modified and create a new set of tests that are just for testing this new setting. |
|
This feature seems non-spec compliant, as OperationIds are required to be document-wide unique values Openapi 3.0 (source https://swagger.io/docs/specification/v3_0/paths-and-operations/)
OpenApi 3.1 (source https://swagger.io/specification/)
|
Thanks for pointing this out @kirides. Most backend tooling should complain when generating a non-compliant spec, and things like SwaggerUI or Scalar won't be able to work with it either. That said, since this is client code generation tool, I'm actually okay with features that are not fully compliant with the OpenAPI specifications, which is also why I use NSwag for parsing the OAS document instead of the Microsoft OpenAPI tooling which is much more strict. NSwag let's you get away with OAS documents that contain errors. I can only hope and assume that the paths, parameters, and schemas are correct in the OAS document are correct, and things like operation ids, tags, summaries, and descriptions can be used as hints for function naming |
3 participants
For #672
IdentifierUtils.Counted()now removes the NSwag appended numbers if parent is not null.RefitMultipleInterfaceByTagGenerator.GenerateCode()now has theinterfaceNameoutside of the op loop scope, after the first in a group, all subsequent op loops it would reset to null, meaning no parent was supplied toIdentifierUtils.Counted().dynamicQuerystringParameterTypehas theinterfaceNameat the start to prevent clashes.Updated
MultipleInterfacesByTagsTests' spec to be able to test these changes.At the moment not all tests pass due to the changes I made to the
dynamicQuerystringParameterTypespecifically, and I would like to put in some tests to ensure thatIdentifierUtils.Counted()does still count correctly when both multiple interfaces by tag generations and for single interface generations.The test that currently fails is
ApizrTests.Generates_Dynamic_Querystring_Parameters_ByTag.Before continuing I was hoping for some feedback to see if this was a suitable method.
From testing and debugging, the numbers are initially appended by NSwag, before the
IdentifierUtils.Counted()method is even reached (hence the stripping of numbers at the end of theoperationIdin my changes). I was hoping to maybe find a way to stop that from happening at that end, but I couldn't figure out exactly where I could do it and saw that there was already a fallback with theIdentifierUtils.Counted()so decided it was suitable to workaround it the way I have done.Since the records for the query params are not within each given interface, a clash can still occur due to the changes I made with
IdentifierUtils.Counted(). I went with prepending them with the Tag used for the interface since the point of my changes was to try and remove the numbers appended to method names, and felt this ideology would then clash with allowing these to still have numbers on the end. But let me know your opinions on whether or not this was the right move or not.If this is the way to go then I would have to change the spec in
ApizrTests.Generates_Dynamic_Querystring_Parameters_ByTagtoo and add in a few more tests like I want to do withMultipleInterfacesByTagsTestsas well.However I'm wondering if this behaviour should be hard coded as I have implemented it so far or be another option in the config.