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

Determine a way to unambiguously re-export types without silent conflicts. #154

Closed
dgramop opened this issue Jan 30, 2022 · 15 comments · Fixed by #452
Closed

Determine a way to unambiguously re-export types without silent conflicts. #154

dgramop opened this issue Jan 30, 2022 · 15 comments · Fixed by #452
Milestone

Comments

@dgramop
Copy link
Contributor

dgramop commented Jan 30, 2022

✔️ @arlyon here! For a fix, please see this comment #154 (comment). This issue is now about determining the best way to fix the aliasing issue.

Hi! I just updated from rc3 to 0.13

I'm trying to create a subscription with SubscriptionPaymentBehavior::DefaultIncomplete

When I pass SubscriptionPaymentBehavior::DefaultIncomplete to CreateSubscription (wrapping inside Option appropriately), the compiler complains that I'm passing the wrong one:

expected enum stripe::resources::generated::billing::subscription::SubscriptionPaymentBehavior, found enum SubscriptionPaymentBehavior

I'm using stripe::SubscriptionPaymentBehavior

If I try using stripe::resources::generated::billing::subscription::SubscriptionPaymentBehavior, the compiler complains that resources is a private module

My features: async-stripe = { version = "0.13", features = ["runtime-tokio-hyper", "webhook-events"] }

@dgramop
Copy link
Contributor Author

dgramop commented Jan 30, 2022

I managed a temporary fix by combining the SubscriptionPaymentBehavior in subscription_item.rs to with SubscriptionPaymentBehavior in subscription.rs

I haven't really wrapped my mind around the code generation yet, so I don't know where to start to contribute a fix back to this repo.

@erichCompSci
Copy link
Contributor

Hey dgramop. They problem is not in the code generation, it's in the exported modules. Code isn't exported automatically in Rust, you have to include the appropriate struct in the correct file (module) to make code visible. However, that means that our code generation could in theory produce duplicates, that people can't find until they show up. I have a very little bit of time, let me take a look and see if I can spot the double code name and figure out a solution for you to try.

@erichCompSci
Copy link
Contributor

Okay, looks like this is a common problem with subscription and subscription_item...if you look into resources.rs you can see that they renamed several of the other modules so that they don't conflict.

The quick fix is to do the same thing for your SubscriptionPaymentBehavior. You do the following:

  1. Fork master
  2. Create a branch called SubscriptionPaymentBehavior-visiblity-fix or something similar
  3. Change resources.rs so that the names for SubscriptionPaymentBehavior in subscription and subscription_item modules don't conflict with the as keyword. You can see this being done as an example around lines 183ish in resources.rs.
  4. Commit, Test, and make a pull request with this repo.

If you run into trouble, push your unfinished work to your forked branch, ping me in this repo and I'll help.

@dgramop
Copy link
Contributor Author

dgramop commented Jan 31, 2022

I was still working on my subscription integration when I found another inconsistency that was causing my expanded Subscription to not deserialize: the generated prefix for invoice line items was wrong - it seems to always be "li_" from stripe.

Would the PR changes I suggest be overwritten if openapi code generation is run again? I had also rolled back to the previous version (0.13.0-rc3), so I'll have to make those changes again.

Additionally, I noticed the version that's on crates.io (0.13.0) is referencing a commit that doesn't seem to exist in this repo: .cargo_vcs_info.json from docs.rs source

git checkout 815db5e4b54e952c539e4841d4cce616b4c7a029
fatal: reference is not a tree: 815db5e4b54e952c539e4841d4cce616b4c7a029

@erichCompSci
Copy link
Contributor

So, a few things.

  1. The only thing that will be overwritten on an openapi code generation is any changes that you make in the generated folder. And you should not make any changes to those files to get something to work for that reason.

  2. The reason the 0.13.0 isn't referencing the right thing is that Arlyon rewrote some history recently on master for some reason or other (I'm not sure why). @arlyon Can you update the crate that's on crates.io so that it's something on the current history?

  3. Finally, yeah, I've discovered that the add_id(String) method doesn't actually work. We should probably open that up as it's own issue.

  4. If you want up to date code, you have to use the github. Arlyon is not paid to maintain this and larger companies with money I think prefer something with more support like python, or something else, so the "releases" are when he can find time and feels like enough work has been done, which takes time.

@arlyon
Copy link
Owner

arlyon commented Jan 31, 2022

Regarding the nonexistent commit, I bumped the verison, published to crates IO, realised the readme changes were not included in the commit (and so the docs on crates io are incorrect) so I amended the commit with the intention to overwrite. Apparently that is not possible, even just for docs changes, and because I chose to amend it locally (we don't have automated releases set up so publishing / versioning is manual atm) the commit was lost. Sorry!

I will push a new version with the changes to default to address this :)

Part two, yes, some places in the openapi generate almost identical structs with similar names that need to be manually exported. This is mainly due to low test coverage (and huge api surface area) so thanks for the report! If you have time to submit a PR I will make sure to merge it asap.

@dgramop
Copy link
Contributor Author

dgramop commented Jan 31, 2022

grep -r "struct " . | sort | uniq -d
grep -r "enum " . | sort | uniq -d

I'm using this to find identically named structs and enums for manual verification that they are indeed duplicates and for merging. I'm currently working on removing these duplicates.

I'm sure there's some way we could have some kind of limited parser that checks fields and enum variants to do this automatically at generation-time, but I'll do it manually for now

@dgramop
Copy link
Contributor Author

dgramop commented Jan 31, 2022

4. f you want up to date code, you have to use the github. Arlyon is not paid to maintain this and larger companies with money I think prefer something with more support like python, or something else, so the "releases" are when he can find time and feels like enough work has been done, which takes time.

Absolutely! I'm familiar enough with how OS works to be very grateful for the time and effort you guys have put into this repo 💛

@dgramop
Copy link
Contributor Author

dgramop commented Jan 31, 2022

I made the PR, and I made another branch in that repo that has a hacky script to remove duplicates (see combine.py)

The python script doesn't really follow some of the standards so well, but it maintains functionality. I tested it with an older cached 0.13.0-rc3 off crates.io (which is what my application uses). I also built it with cargo make openapi-install and adjusted the Makefile.toml accordingly

I'm new to this build process so please let me know if I missed anything!

@SorenHolstHansen
Copy link

Any progress on this, am running in to the same issue

@arlyon
Copy link
Owner

arlyon commented Feb 20, 2022

Hi! There is a PR to try and dedupe a number of types but I haven't had a chance to get round to reviewing it. In the mean time, you can clone the repo and work locally, following the pattern for subscription and subscription_item in src/resources.rs to manually export the conflicting types :)

subscription_item::PlanInterval as SubscriptionItemInterval,
subscription_item::SubscriptionItemPriceDataRecurring as SubscriptionItemPriceDataRecurring,
subscription_item::SubscriptionItemPriceData as SubscriptionItemPriceData,
// need to import this afterwards so that the SubscriptionItemPriceDataRecurring
// isn't silently ignored
subscription::*,
subscription::PlanInterval as SubscriptionInterval,
subscription::SubscriptionItemPriceDataRecurring as SubscriptionPriceDataRecurring,
subscription::SubscriptionItemPriceData as SubscriptionPriceData,

@moatra
Copy link

moatra commented Nov 7, 2022

Just ran into this myself. For users of the library, you can hack/workaround it by letting the compiler infer the type when deserializing the equivalent json.

let mut to_create = CreateSubscription::new(customer);
// async-stripe just reexports the generated openapi structs, and sometimes they have duplicated names.
// hack: let the compiler infer which one to use.
let behavior = serde_json::from_str("default_incomplete").unwrap();
to_create.payment_behavior = Some(behavior);

For a fix on the library side, it sounds like reviewing the renaming PR has been pending for a while. Would it be a reasonable smaller fix to make the stripe::resources::generated module hierarchy public so that the users can import the specific, fully-qualified struct from there?

@arlyon
Copy link
Owner

arlyon commented Nov 11, 2022

I think that that is a fair request. The PR stalled because we haven't settled on a heuristic for deciding how to deduplicate the types. I think we will likely just need to export both under different types. We are currently bringing order to the mess that is the codegen and will look at refactoring to rename things such that they are distinct but in the mean time exporting the generated module is a decent (non-breaking) stopgap.

@arlyon
Copy link
Owner

arlyon commented Nov 11, 2022

Partially addressed in #296 which exports generated such that you can access any type by its full path rather than using the types that are exported. By default, rust shadows types which is not what you want in this case, but importing it using the full path is guaranteed to work.

@arlyon arlyon changed the title SubscriptionPaymentBehavior in crate root isn't the same as the one for Create Subscription Determine a way to unambiguously re-export types without silent conflicts. Nov 17, 2022
@arlyon
Copy link
Owner

arlyon commented Apr 4, 2024

This is partially addressed by the above but also rustc now warns on these.

@arlyon arlyon closed this as completed Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment