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

Duplicate operation Ids between PATCH and PUT when both are supported #600

Closed
baywet opened this issue Oct 23, 2024 · 5 comments · Fixed by #601
Closed

Duplicate operation Ids between PATCH and PUT when both are supported #600

baywet opened this issue Oct 23, 2024 · 5 comments · Fixed by #601
Assignees
Labels
priority:p1 High priority but not blocking. Causes major but not critical loss of functionality SLA <=7days type:bug A broken experience type:regression A bug from previous release

Comments

@baywet
Copy link
Member

baywet commented Oct 23, 2024

follow up to #596 and #594

Happens in beta, but will eventually roll out to v1

PUT /servicePrincipals/{servicePrincipal-id}/claimsPolicy
PATCH /servicePrincipals/{servicePrincipal-id}/claimsPolicy

Both have the same servicePrincipals.UpdateClaimsPolicy operation id and they should not.

@baywet baywet added priority:p1 High priority but not blocking. Causes major but not critical loss of functionality SLA <=7days type:bug A broken experience type:regression A bug from previous release labels Oct 23, 2024
@irvinesunday
Copy link
Contributor

irvinesunday commented Oct 24, 2024

PowerShell uses these two operation ids but distinguishes them by changing the PUT operation id from servicePrincipals.UpdateClaimsPolicy to servicePrincipals_SetClaimsPolicy. This happens via DevX API during slicing of OpenAPI for PowerShell. Barring the underscore, I wonder if we can rename all PUT operations to xxx.Setyyy ?

@baywet
Copy link
Member Author

baywet commented Oct 24, 2024

As far as I know, PowerShell is the only surface we have that depends on OperationIds. Copilots with Microsoft plugins manifest also use them, but when the manifest is generated from the description, the operation id gets copied and normalized. So any update should flow through.
Can you validate that changing the source won't break PowerShell downstream before proceeding to the change please?

@irvinesunday
Copy link
Contributor

This won't affect PowerShell, because as I mentioned before, we ultimately transform PUT operationIds to use .SetXxx instead of .UpdateXxx via DevX API before consumption by AutoREST.

@timayabi2020 to confirm that this assertion is true, and that the proposed change will not affect PowerShell.

@timayabi2020
Copy link
Contributor

timayabi2020 commented Oct 25, 2024

@irvinesunday yes it is true, with that transformation all PUT operations will translate to cmdlets with the verb Set, while PATCH operations will have cmdlets starting with the verb Update.

Using the latest Graph PowerShell installed in your machine, you can confirm the assertion by running the either of these commands.
Find-MgGraphCommand -Command Update-.* | Select Method, Command
Find-MgGraphCommand -Command Set-.* | Select Method, Command

@irvinesunday irvinesunday self-assigned this Oct 25, 2024
@baywet
Copy link
Member Author

baywet commented Oct 25, 2024

Great! Thanks everyone for triple checking before we make some changes. @irvinesunday good to proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p1 High priority but not blocking. Causes major but not critical loss of functionality SLA <=7days type:bug A broken experience type:regression A bug from previous release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants