refactor: change swagger operationId output

[compojoom]

Summary

I'm using RTK code generation and the generated code had very ugly names. Here is an example:

I'm getting a lot of java vibes here :) Those method names are auto-generated based on the operationId.

I think that chances of collision are small if we remove the controller from the operationId.
It seems that you guys follow the convension:
AccountsController

• createAccount
• getAccount..
• upsertAccount
• deleteAccount

so despite the fact that I would remove accountsController of the name I won't have a similar operationId elsewhere as you are adding the controler name in the function

With this approach we end up with code like this:

WDYT?

@iamacook is this change going to be problematic for your client-gateway-typescript-sdk?

[hectorgomezv]

For reference, this is the complete list of operationId I get with this approach on the current codebase:

"operationId": "GetAbout",
"operationId": "v1AddConfirmation",
"operationId": "v1CheckEligibility",
"operationId": "v1CreateMessage",
"operationId": "v1CreateSubmission",
"operationId": "v1DeleteDelegate",
"operationId": "v1DeleteSafeDelegate",
"operationId": "v1DeleteTransaction",
"operationId": "v1GetAboutChain",
"operationId": "v1GetAllSafesByOwner",
"operationId": "v1GetBackbone",
"operationId": "v1GetBalances",
"operationId": "v1GetCampaignActivities",
"operationId": "v1GetCampaignById",
"operationId": "v1GetCampaignLeaderboard",
"operationId": "v1GetCampaignRank",
"operationId": "v1GetCampaigns",
"operationId": "v1GetChain",
"operationId": "v1GetChains",
"operationId": "v1GetContract",
"operationId": "v1GetCreationTransaction",
"operationId": "v1GetDataDecoded",
"operationId": "v1GetDelegates",
"operationId": "v1GetIncomingTransfers",
"operationId": "v1GetIndexingStatus",
"operationId": "v1GetLeaderboard",
"operationId": "v1GetLockingHistory",
"operationId": "v1GetLockingRank",
"operationId": "v1GetMasterCopies",
"operationId": "v1GetMessageByHash",
"operationId": "v1GetMessagesBySafe",
"operationId": "v1GetModuleTransactions",
"operationId": "v1GetMultisigTransactions",
"operationId": "v1GetNonces",
"operationId": "v1GetRelaysRemaining",
"operationId": "v1GetSafe",
"operationId": "v1GetSafeApps",
"operationId": "v1GetSafeOverview",
"operationId": "v1GetSafesByOwner",
"operationId": "v1GetSubmission",
"operationId": "v1GetSupportedFiatCodes",
"operationId": "v1GetTransactionById",
"operationId": "v1GetTransactionConfirmationView",
"operationId": "v1GetTransactionQueue",
"operationId": "v1GetTransactionsHistory",
"operationId": "v1PostDelegate",
"operationId": "v1PreviewTransaction",
"operationId": "v1ProposeTransaction",
"operationId": "v1RegisterDevice",
"operationId": "v1Relay",
"operationId": "v1UnregisterDevice",
"operationId": "v1UnregisterSafe",
"operationId": "v1UpdateMessageSignature",
"operationId": "v2GetCollectibles",
"operationId": "v2GetEstimation",

It looks good to me, but we would need to be cautious as we can't reuse method names through controllers, as the controller name would not be part of the operationId anymore @iamacook @PooyaRaki 🙂

[katspaugh]

👍

[iamacook]

@compojoom, amazing work!

This will break the SDK generation, but equally allow simplifying it. I have two questions but am otherwise happy to progress with this/fix the generation:

1. I'm a bit concerned about potential naming conflicts, especially since these could exist across different controllers. Do you know if duplicate names will cause errors or if there's a safeguard in place for that?
2. While we currently don’t have versioned methods like this, I feel getExampleV1 and getExampleV2 might be a bit clearer than V1GetExample and V2GetExample. What do you think about moving the version to the end?

[katspaugh]

Just my 50 cents regarding V1 prefixes: I find them ugly but they will make it easier to migrate the web app because you'll see immediately which types are new and which are old. In this regard, a prefix notation is better than postfix.

[compojoom]

Maybe we should keep the controller name and just remove "controller" string from it?

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

https://github.com/OAI/OpenAPI-Specification/blob/main/versions/2.0.md#fixed-fields-5

Having the version number at the end felt weirder for me than having it at the begining, but I'm not overly attached to that. Will move it to the end if it makes more sense for you.

[compojoom]

Just my 50 cents regarding V1 prefixes: I find them ugly but they will make it easier to migrate the web app because you'll see immediately which types are new and which are old. In this regard, a prefix notation is better than postfix.

I also found it ugly ,but now that I think of it. If we have 2 operationIds
getSafesV1
getSafesV2

your IDE should offer you autocomplete and you'll see that there is a new v2 version when you start typing "getS"...
If we do it the way I implemented it, since one would be used to typing V1get -> you would get only the proposal for the V1 method and wouldn't know that there is a new endpoint 🤔

[iamacook]

Maybe we should keep the controller name and just remove "controller" string from it?

I think it's fine as is but could lead to sneaky bugs in the future. Do you know if there's an error if a conflict happens?

Having the version number at the end felt weirder for me than having it at the begining, but I'm not overly attached to that. Will move it to the end if it makes more sense for you.

The current SDK implementation generates versions at the end. However, I have no strong opinion as these are used clientside.

I'd suggest going with what you prefer and we can then reduce complex name generation from the SDK generation accordingly.

[compojoom]

I think it's fine as is but could lead to sneaky bugs in the future. Do you know if there's an error if a conflict happens?

Yes, checked it. It errors out.

> [email protected] api-generate
> npx @rtk-query/codegen-openapi openapi-config.ts

Generating ./src/store/gateway/AUTO_GENERATED/about.ts
Done
Generating ./src/store/gateway/AUTO_GENERATED/accounts.ts
Error: interface/type alias V1CreateAccountApiResponse already registered

Ok, I'll modfiy the operationId to be controllerName_methodName_version

[iamacook]

Ok, I'll modfiy the operationId to be controllerName_methodName_version

Is this then how you would want the wrappers to be named in the SDK, or remain as is?

[compojoom]

Ok, here are the generated operationIds with my latest changes:

aboutGetAbout
accountsCreateAccountV1
accountsGetDataTypesV1
accountsGetAccountDataSettingsV1
accountsUpsertAccountDataSettingsV1
accountsGetAccountV1
accountsDeleteAccountV1
addressBooksGetAddressBookV1
addressBooksCreateAddressBookItemV1
addressBooksDeleteAddressBookV1
addressBooksDeleteAddressBookItemV1
counterfactualSafesGetCounterfactualSafeV1
counterfactualSafesGetCounterfactualSafesV1
counterfactualSafesCreateCounterfactualSafeV1
counterfactualSafesDeleteCounterfactualSafeV1
counterfactualSafesDeleteCounterfactualSafesV1
authGetNonceV1
authVerifyV1
balancesGetBalancesV1
balancesGetSupportedFiatCodesV1
chainsGetChainsV1
chainsGetChainV1
chainsGetAboutChainV1
chainsGetBackboneV1
chainsGetMasterCopiesV1
chainsGetIndexingStatusV1
collectiblesGetCollectiblesV2
communityGetCampaignsV1
communityGetCampaignByIdV1
communityGetCampaignActivitiesV1
communityGetCampaignLeaderboardV1
communityGetCampaignRankV1
communityCheckEligibilityV1
communityGetLeaderboardV1
communityGetLockingRankV1
communityGetLockingHistoryV1
contractsGetContractV1
dataDecodedGetDataDecodedV1
delegatesGetDelegatesV1
delegatesPostDelegateV1
delegatesDeleteDelegateV1
delegatesDeleteSafeDelegateV1
delegatesGetDelegatesV2
delegatesPostDelegateV2
delegatesDeleteDelegateV2
estimationsGetEstimationV2
messagesGetMessageByHashV1
messagesGetMessagesBySafeV1
messagesCreateMessageV1
messagesUpdateMessageSignatureV1
notificationsRegisterDeviceV1
notificationsUnregisterDeviceV1
notificationsUnregisterSafeV1
ownersGetSafesByOwnerV1
ownersGetAllSafesByOwnerV1
relayRelayV1
relayGetRelaysRemainingV1
safeAppsGetSafeAppsV1
safesGetSafeV1
safesGetNoncesV1
safesGetSafeOverviewV1
targetedMessagingGetSubmissionV1
targetedMessagingCreateSubmissionV1
transactionsGetTransactionByIdV1
transactionsGetMultisigTransactionsV1
transactionsDeleteTransactionV1
transactionsGetModuleTransactionsV1
transactionsAddConfirmationV1
transactionsGetIncomingTransfersV1
transactionsPreviewTransactionV1
transactionsGetTransactionQueueV1
transactionsGetTransactionsHistoryV1
transactionsProposeTransactionV1
transactionsGetCreationTransactionV1
transactionsViewGetTransactionConfirmationViewV1

You can view here the generated RTK query code: https://github.com/safe-global/safe-wallet-mobile/pull/33

One small annoyance is that the exported RTK hooks, have the Query suffix, so we end up with hooks names like:
useTransactionsProposeTransactionV1Query
useTransactionsGetCreationTransactionV1Query

I think that we will get used to it.

[iamacook]

LGTM.

When this is merged, it will beak the SDK generation. If this isn't urgent, I'd suggest merging it Friday and then fixing the aforementioned.

Are we happy to use these names as the "wrappers" there as well? It will make things a lot simple codewise.

[compojoom]

Fine with me. I already generated the code for the mobile app baased on this PR.