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 for getting account by id #786

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

maartenvanvliet
Copy link
Member

Hey 👋

I added a way to override the method_name for an operation. These are not unique per schema apparently.
Did some cursory exploration on how other stripe sdk's deal with this:

  • php - the Account.retrieve without an id is not implemented
  • node - special cased, Account.retrieve handles the case where id is null.
  • ruby - special cased implementation, overrides the parent class' implementation
  • go - adds a getById method.

I've gone with the go way of fixing this and added retrieve_by_id. Two other resources had the same issue but naming was more straightforward there.

Also, the method_on: "collection" Stripe operations are now skipped. They were duplicates of the "service" operations and I think only used for code generation by Stripe itself.

fixes #777

@nkezhaya
Copy link
Contributor

@maartenvanvliet Thanks for working on this! Maybe it's just me, but retrieve_by_id seems less appealing than retrieve.

Is there a benefit to having an Account.retrieve/0 function? It should just return an error, no?

@maartenvanvliet
Copy link
Member Author

I agree that it's aesthetically less pleasing to have this one exception to how it's done the sdk and naming this function retrieve_by_id.
Other options I've considered were to name Account.retrieve/0 => Account.get/0 or Account.show/0, which are also not great imo. Because the go-lang implementation went this way, I followed suit.

The Account.retrieve/0 function will return the results for your own account, so it serves a purpose there. Though this information can also be retrieved with the Account.retrieve/1 function if you know the account id.

I'm not particularly attached to any function name and think something can be said for all of them. If there's consensus on the issue I'll push the changes.

@nkezhaya
Copy link
Contributor

Oh right, my mistake!

I'm not a go expert by any means, but I suspect that GetByID might better follow go naming conventions. I could see an argument for get() and get(id) because users might be used to it from using Ecto and other libraries, but there'd be a small burden of changing all instances of retrieve/* to get/*.

Personally, I don't see anything wrong with Resource.retrieve() and Resource.retrieve(id).

@maartenvanvliet
Copy link
Member Author

I mentioned that the two operations are not uniquely named in the schema but have failed to highlight the underlying issue. Thing is we cannot support retrieve/0-2 and retrieve/1-3 at the same time. The code generated leads to a compile issue where the defaults of the two functions clash. If this were possible it would be my preferred solution.

The simplest fix is to rename one of those functions to something else, that's why the overrides are there. The complicated fix is to have the generated code handle the different cases. This would imo hamper maintainability of the code for something that has 3 occurrences in the entire Stripe open api schema.

In light of this discussion, I propose to go with Account.show/0 for your own account, and keep Account.retrieve/1 to fetch another account.

@nkezhaya
Copy link
Contributor

It's been a couple days and I'm unable to come up with anything better. 🙂 I like Account.show/0!

@maartenvanvliet
Copy link
Member Author

@yordis I'll try to find some time to fix the conflicts and address the comments so this can be merged.

@yordis
Copy link
Member

yordis commented Aug 21, 2023

Sorry to be late to the party!

I am a bit confused. What is the difference between get_by_id/1 and retrieve/1?

Purely looking based on the test, Stripe.Account.retrieve("acct_123") should work, no?

I am used to Elixir leveraging pattern matching to avoid function explosion, so having a function that accepts a string or an object and things like that is expected.

@maartenvanvliet
Copy link
Member Author

The issue is that the automatically generated functions clash in a few cases because of arity and defaults. In this case you'll get def retrieve/3 defaults conflicts with retrieve/2. That's why a method to override function names was introduced to bypass the clash.

@maartenvanvliet maartenvanvliet requested a review from a team as a code owner August 21, 2023 19:44
@yordis
Copy link
Member

yordis commented Aug 21, 2023

CI failing, overall looking good 🚀 💜

@github-actions
Copy link

This pull request has been automatically marked as "stale:discard". If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated!.

@gmrcp
Copy link

gmrcp commented Jan 26, 2024

Hey 👋 do you have any updates on this?
I'm happy to help on the failing CI to push this forward

@yordis
Copy link
Member

yordis commented Jan 26, 2024

@gmrcp please do 🙏🏻

@maartenvanvliet
Copy link
Member Author

CI failures are fixed, I also fixed an unrelated CI failure that was erroring with query params.

@yordis
Copy link
Member

yordis commented Feb 1, 2024

Should this be a breaking change? 🤔

@gjsduarte
Copy link

gjsduarte commented Jun 6, 2024

Hey everyone,
I'm really looking forward to this PR being merged.
Is there anything I can do to help you do so?

@maartenvanvliet, is there any chance you can resolve the conflicts?

@cgarvis
Copy link

cgarvis commented Aug 29, 2024

@yordis anyway we can get this merged in?

@yordis
Copy link
Member

yordis commented Sep 4, 2024

@cgarvis I have a #853 where we must manually add ALL mapping. I do not want to make assumptions anymore; everything is explicit until we fix the anti-pattern (we should flatten the codegen and use operation ID as identity).

I would take any help with mix stripe.generate with the proper version (see makefile). It is a lot of grunt work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stripe.Account.retrive/1 not working in OpenAPI build
6 participants