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

fix(): use unique operation id for each endpoint (class_method) #487

Merged
merged 1 commit into from
Jan 18, 2020

Conversation

kamilmysliwiec
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Operation ID == method name

Issue Number: #482

What is the new behavior?

Operation ID == ClassName_MethodName

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

};
}

private getOperationId(instance: object, method: Function): string {
if (instance.constructor) {
return `${instance.constructor.name}_${method.name}`;
Copy link

@boenrobot boenrobot Jan 14, 2020

Choose a reason for hiding this comment

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

As much as I need this fix too, wouldn't it be better if an invalid character in a JS identifier is used as a separator, to prevent creating unintentional clashes?

Like "-" for example? Generators should be smart enough to escape that into the target language, and doc generators can generate URLs without requiring URL escaping even.

Example of a clash:

ModuleController_UsingThisCase controller
DoAction method

vs.
ModuleController controller
UsingThisCase_DoAction method

sure, chances of such collisions happening in the same code base are minimal, at best, given the popularity of linting tools, but still. "-" is not valid in a JS identifier, so one can't possibly cause a clash with the defaults alone.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if we should protect ourselves against code that shouldn't appear in any codebase since it simply violates too many rules (+ probability of having such a naming clash can be brought down to 0)

@kamilmysliwiec kamilmysliwiec merged commit a323bb0 into master Jan 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix/unique-operation-id branch January 18, 2020 08:59
@Syarx
Copy link

Syarx commented Feb 20, 2020

This is a breaking change. You should give an option to disable it. Some of us have to rewrite hundreds of calls now. I have to downgrade until then sadly

dennisameling added a commit to dennisameling/swagger that referenced this pull request May 16, 2020
Make the behavior change for operation names that was introduced in nestjs#487 configurable
@lock
Copy link

lock bot commented May 20, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants