Skip to content

Conversation

@alolis
Copy link

@alolis alolis commented Sep 7, 2025

  • Updated Phases.Compile to properly sanitize generated type names that collide with reserved words
  • Updated Phases.Compile operations to be compatible with the OpenApi SDK
  • Updated default @api_version to 2025-08-27.basil

Reference: https://github.com/stripe/openapi/releases/tag/v1941

* Updated Phases.Compile to properly sanitize generated type names that collide with reserved words
* Updated Phases.Compile operations to be compatible with the OpenApi SDK
* Updated default @api_version to 2025-08-27.basil

Reference: https://github.com/stripe/openapi/releases/tag/v1941
@alolis alolis requested a review from a team as a code owner September 7, 2025 13:46
@yordis
Copy link
Member

yordis commented Sep 7, 2025

@alolis btw, if the tests fail, the intent is to "try" to keep the existing APIs without breaking changes, if possible, as much as we can

@alolis
Copy link
Author

alolis commented Sep 7, 2025

@alolis btw, if the tests fail, the intent is to "try" to keep the existing APIs without breaking changes, if possible, as much as we can

Ι will have a look and try to understand what is failing and try to fix it. I will get back to you soon.

@yordis
Copy link
Member

yordis commented Sep 7, 2025

the idea of that ugly mapping was to power-up thru manual work to make sure the mapping is fixing the breaking change

mjcloutier added a commit to ineedthis/stripity-stripe that referenced this pull request Oct 13, 2025
…o Subscription struct

The OpenAPI spec v1941 merged from PR beam-community#874 is missing current_period_end
and current_period_start fields in the subscription schema. These are core
fields that Stripe returns in subscription objects.

This fix manually adds:
- current_period_end: integer - End of current billing period
- current_period_start: integer - Start of current billing period

Both to the defstruct, @type spec, and @TypeDoc documentation.
@github-actions
Copy link

github-actions bot commented Nov 7, 2025

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!.

@alolis
Copy link
Author

alolis commented Nov 7, 2025

bump

@yordis
Copy link
Member

yordis commented Nov 7, 2025

@alolis are you waiting for me btw?

@alolis
Copy link
Author

alolis commented Nov 7, 2025

@alolis are you waiting for me btw?

My apologies, no, I just I had an extremely huge workload and that's why I never pushed. I will deal with it soon.

@zubairshokh
Copy link
Contributor

Can we have any update on this?

@Xunjin
Copy link

Xunjin commented Dec 1, 2025

Can we have any update on this?

I could help out @alolis if you are time limited.

@alolis
Copy link
Author

alolis commented Dec 1, 2025

I have finally found some time to deal with this. Give me until Wednesday to try and push updates and If i need help I will let you know no matter what.

@alolis
Copy link
Author

alolis commented Dec 2, 2025

@yordis , so, there are 13 tests failing and I have been trying to figure out what is going on. I will just give you one example to make things a bit simpler in order for you to guide me a bit if you can.

This PR, from my understanding, has generated the modules for Stripe API version v1941 (aka 2025-08-27.basil) as also indicated in spec3.sdk.json, line 77309

  "info": {
    "contact": {
      "email": "dev-platform@stripe.com",
      "name": "Stripe Dev Platform Team",
      "url": "https://stripe.com"
    },
    "description": "The Stripe REST API. Please see https://stripe.com/docs/api for more details.",
    "termsOfService": "https://stripe.com/us/terms/",
    "title": "Stripe API",
    "version": "2025-08-27.basil",
    "x-stripeSpecFilename": "spec3.sdk"
  },

I run the following test:

mix test test/stripe/payment_methods/bank_account_test.exs

and I get


1) test update/2 updates a bank account (Stripe.BankAccountTest)
   test/stripe/payment_methods/bank_account_test.exs:5
   Assertion with == failed
   code:  assert expected_url == url
   left:  "http://localhost:12111/v1/customers/cus_123/sources/ba_123"
   right: "http://localhost:12111/v1/accounts/cus_123/external_accounts/ba_123"
   stacktrace:
     (stripity_stripe 3.2.0) test/support/stripe_case.ex:16: Stripe.StripeCase.assert_stripe_requested/3
     test/stripe/payment_methods/bank_account_test.exs:7: (test)


15:14:33.763 [warning] Extra keys were received but ignored when converting Stripe.BankAccount: [:deleted]


2) test delete/2 deletes a bank account (Stripe.BankAccountTest)
   test/stripe/payment_methods/bank_account_test.exs:12
   Assertion with == failed
   code:  assert expected_url == url
   left:  "http://localhost:12111/v1/customers/cus_123/sources/ba_123"
   right: "http://localhost:12111/v1/accounts/cus_123/external_accounts/ba_123"
   stacktrace:
     (stripity_stripe 3.2.0) test/support/stripe_case.ex:16: Stripe.StripeCase.assert_stripe_requested/3
     test/stripe/payment_methods/bank_account_test.exs:14: (test)

The reason is that in the generated BankAccount, if you search for def update and def account you will see that the endpoints are different (they are prefixed with /v1/accounts instead of /v1/customers). But if you go to the official API docs for the API version I used, you can see that the url from the test is indeed correct.

Any idea why this might be happening so I can check it out?

@alolis
Copy link
Author

alolis commented Dec 2, 2025

@yordis I think I have used an older release tag for the basil version. Checking and I will get back to you shortly.

@Xunjin
Copy link

Xunjin commented Dec 2, 2025

@yordis I think I have used an older release tag for the basil version. Checking and I will get back to you shortly.

@alolis
I think the version you are looking for is v2017, as in v2018 the version changes to the first iteration of clover.

stripe/openapi@7861d49#diff-1944e58fe19446ee7dce4dee729f100816214ac4fdd37d86f864e8e7c1437fa8R78325.
my bad, in the end they have the same checksum:

curl https://raw.githubusercontent.com/stripe/openapi/v1941/openapi/spec3.sdk.json | sha256sum
112322c4eaa41183c8ce409090c9a556b2950f8586ac5cfd3909cedb6416b5bd

curl https://raw.githubusercontent.com/stripe/openapi/v2017/openapi/spec3.sdk.json | sha256sum
112322c4eaa41183c8ce409090c9a556b2950f8586ac5cfd3909cedb6416b5bd

* Updated generated files
* Updated compile phase to properly escape operation definitions
@alolis
Copy link
Author

alolis commented Dec 2, 2025

@Xunjin I updated it anyway but I do not think that's the issue after all. It probably needs some further patching in compile.ex. Not sure if I am the appropriate person to do this. I did try a few things in the last few hours but someone with deeper knowledge of the library probably has to take a look at it.

@yordis
Copy link
Member

yordis commented Dec 2, 2025

@alolis the people had deep knowledge are gone by now, so you and I are left to figure out things together.

I added the ugly mapping

# NOTE: This is a mapping of operation ids to function names. This is necessary
# to avoid breaking changes. In the future, we should remove this mapping and
# use the operation id directly as the function name.
#
# Temporarily, I am adding all the existing ones although using
# stripe_extension["method_name"] could be default while we only add new ones.
# That will be done in follow up work since it would be difficult to assess
# the situation.
@operation_identity_mapping %{

Simply to manually remap what suppose to be the non-breaking change version since some operations overlap now, and decide a new name for the new operation.

Sadly, the Stripe SDK has these polymorphic endpoints that they decided based on the inputs or something like that, which we aren't following.

@Xunjin
Copy link

Xunjin commented Dec 2, 2025

@alolis the people had deep knowledge are gone by now, so you and I are left to figure out things together.

I added the ugly mapping

# NOTE: This is a mapping of operation ids to function names. This is necessary
# to avoid breaking changes. In the future, we should remove this mapping and
# use the operation id directly as the function name.
#
# Temporarily, I am adding all the existing ones although using
# stripe_extension["method_name"] could be default while we only add new ones.
# That will be done in follow up work since it would be difficult to assess
# the situation.
@operation_identity_mapping %{

Simply to manually remap what suppose to be the non-breaking change version since some operations overlap now, and decide a new name for the new operation.

Sadly, the Stripe SDK has these polymorphic endpoints that they decided based on the inputs or something like that, which we aren't following.

Ohh ok, maybe the mapping would solve the issue that we have right now?

While I'm LF a new job I could try to figure out these deep/hard parts?

Tbh I'm not an expert in metaprogramming Elixir, however this can be a good opportunity to learn more 😅

@yordis
Copy link
Member

yordis commented Dec 2, 2025

That ugly mapping is so that we map to the existing operation back to what was there before, a lot of grunt work, 😭

defp to_func_name(operation, stripe_extension) do
# NOTE: Well, Operation ID suppose to be unique, but it's not. So, we need to use a tuple to make it unique.
key = {operation.id, operation.path, stripe_extension["method_name"]}
case Map.get(@operation_identity_mapping, key) do
nil ->
suggested_mapping = ~s(#{inspect(key)} => "#{stripe_extension["method_name"]}",\n)
raise """
Unregistered Operation
- Stripe Extension: #{inspect(operation)}
- Operation: #{inspect(operation)}
- Suggested Mapping: "#{suggested_mapping}"
"""
# TODO: Fallback to old mapping when the operation is not registered.
# Waiting until I can successfully codegen newer versions of the spec.
# stripe_extension["method_name"]
name ->
name
end

That is intentionally forcing us to figure out the backwards compatible, we can not rely on the order or whatever it is in the OpenAPI spec since it wouldn't guarantee the correctness sadly

@alolis
Copy link
Author

alolis commented Dec 2, 2025

@yordis I took a more aggressive approach :)

  1. I have upgraded my local stripe-mock to latest stripe-mock 0.197.0
  2. I have upgraded stripity-stripe to LATEST v2129 (2025-11-17.clover)
  3. Updated @operation_identity_mapping with the new Stripe functions and re-generated everything
  4. Fed the library to Claude Sonnet 4.5 and by providing the latest OpenAPI Stripe spec as context (and a lot of tests output) I requested from Claude to go through all tests and cleanup incompatible ones based on the Spec and update tests that use older endpoints

It needs further checks of course but I do not think that it's possible to keep everything backwards compatible. Stripe keeps changing things and tests will keep failing. I would probably solve this by changing the major version of the library every time a new major Stripe version was released. Anything else is probably pain.

What do you think?

@Xunjin The map was ok with the previous version, I did have to update it now that i have done the above.

@yordis
Copy link
Member

yordis commented Dec 2, 2025

@alolis let me rephrase it, the existing functions must point to what was there before, we can not call a new endpoint.

My intent is to try* to avoid breaking changes as much as we can so upgrading isn't as painful, but I can't guarantee anything if Stripe changed things for sure

@Xunjin
Copy link

Xunjin commented Dec 2, 2025

@alolis let me rephrase it, the existing functions must point to what was there before, we can not call a new endpoint.

My intent is to try* to avoid breaking changes as much as we can so upgrading isn't as painful, but I can't guarantee anything if Stripe changed things for sure

Based on this approach, which I do agree with, we will never really ditch the mapping, ugly or not, is honest work ™. Therefore, to keep the library up to date would require being some versions behind and deciding case by case, which would require the mapping for backwards compatibility reasons. However, this requires (already) a ton of work.

Maybe it's time to test moving forward with all that's discussed here: #869

wdyt? @yordis

@alolis
Copy link
Author

alolis commented Dec 2, 2025

I understand @yordis but I do not think the current approach is viable. The latest version of the library is basically not working at the moment since Stripe API has moved miles and miles ahead since v755.

I agree with @Xunjin. The proposed approach in #869 is the way forward and from what I am reading in the comments, you agree. From what I understand the other communities are doing the same approach as well (e.g. stripe-ruby). A little bit of breakage now to ensure that the library will be easily maintainable in the future does not sound bad to me. Besides, no matter what you do, 100% backwards compatibility is not possible, even if you pin the API version. You will have to pin the library version as well. At some point, as a Striper API consumer, you will have to deal with Stripe API changes no matter what.

What was the reason you never moved forward with it back then? Time or you just wanted to keep the backwards compatibility as long as possible?

If you do move forward with the OpenAPI generator I believe this PR should be closed.

@yordis
Copy link
Member

yordis commented Dec 2, 2025

At some point, as a Striper API consumer, you will have to deal with Stripe API changes no matter what.

I do not disagree here for sure. I do not feel strong about anything, if we are gonna reset the situation, I am all for it.

I am not sure about open_api_generator as the solution neither. After I have written so many OpenAPI codegen, I have few strong opinions that I need to validate before, personally, I would be onboard with that package.

@zubairshokh
Copy link
Contributor

zubairshokh commented Dec 12, 2025

Hello,

It feels like a big part of the delay is trying to keep full backward compatibility with the older behavior. Just a suggestion — since this is already a major upgrade, it might be worth reconsidering how much compatibility we actually need to preserve.

Allowing some breaking changes might help unblock this and get the newer Stripe API support merged sooner.
Let’s keep this PR moving by focusing on getting the upgrade merged, even if it introduces breaking changes. A major version bump exists for exactly this reason — we shouldn’t hold back the entire library for legacy behavior.

Pushing this forward is more valuable than keeping an aging compatibility layer alive, just wanted to share that perspective in case it helps move things forward

@zubairshokh
Copy link
Contributor

Hello,

Just following up on this. Do we have any update on this?
I have been using this PR locally for most of my billing transactions, it works without any issue.
Can we have final verdict on this PR?
Thanks

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants