alexschimpf / fastapi-versionizer

FastAPI Versionizer
MIT License
81 stars 13 forks source link

Support for {patch} level version digit to make possible YYYY-MM-DD version formats #2

Open ndejong opened 1 year ago

ndejong commented 1 year ago

Hi -

I'd like to discuss support for the third SEMVER digit in the version format

If we make the patch level digit possible it is then possible to use versioning schemes that use the date in the same way that Amazon, Stripe and others do.

For example /2023-01-12/users/awesome where the instantiation of that might look something like

versions = versionize(
    app=app,
    prefix_format='/{major}-{minor:02}-{patch:02}',
    enable_latest=True,
    latest_prefix='/latest'
)

The goal is to create a pathway to adopt Stripe style versioning for users - https://stripe.com/blog/api-versioning

Happy to write this as supply a PR, but wanted to ask first to make sure there are not other approaches or limitations that others might see beforehand

alexschimpf commented 1 year ago

Hmm, in my opinion, you shouldn't ever have 2 accessible versions of a route, where they only differ in patch version. When the patch version is incremented, the change should be backwards compatible. So I can't really think of a reason why you'd want versioning to that level. Major version changes should be backwards compatible as well, but there is probably a stronger argument for wanting versioning at that level, since there is new functionality being added.

While the "YYYY-MM-DD" date format does conveniently fit the semantic versioning format, I feel like Stripe's intention is that the date is more of a single major version. They specifically say: "Although backwards-incompatible, each one contains a small set of changes that make incremental upgrades relatively easy so that integrations can stay current.". To me, this means it's a major version.

ndejong commented 1 year ago

Yes, I can agree with your position.

Do we have any other pathway that would open up the ability to use three levels of versions that in-turn makes date style versioning possible?

It's less about being strictly "right" than making a common API versioning format possible

alexschimpf commented 1 year ago

How about this?

@api_version('2023-01-12')
@app.get('/something')
def get_something():
   ...

To me, it feels like they're just using a date as a major version, instead of a single number.

ndejong commented 1 year ago

Perhaps the answer is to simply hardcode the a date with an internal version

versions = versionize(
    app=app,
    prefix_format='/2023-01-12',
    enable_latest=True,
    latest_prefix='/latest'
)
alexschimpf commented 1 year ago

Oh shoot, I just realized I have major/minor typed as ints in the decorator. Didn't realize that. I suppose I can make them be int | str.

alexschimpf commented 1 year ago

Okay, I just went to implement this but then realized it's more complicated than originally thought.

When the versionize function runs, it sorts the versions in ascending order. By doing that, it's able to determine which endpoints stay and which get overridden for each version.

If I allow for arbitrary strings, then that sort won't always do as expected. There is no easy way to distinguish which versions are older or newer, like you can with ints. In your case of dates, the sorting should work as expected (as long as you always follow "YYYY-MM-DD" format). You will just get type-checker warnings, if you even use one. You will also probably need to set the default_version param of versionize to be a tuple of strings.

Here is a potential solution: You must always give major/minor versions as ints. But you can also give a string alias for each version.

So in you're case, you'd have something like:

@api_version(1, alias='2023-01-12')
@app.get('/something')
def get_something():
   ...

@api_version(2, alias='2023-02-05')
@app.get('/something')
def get_something_2():
   ...

which produces the following routes...

GET /2023-01-12/something
GET /2023-02-05/something

However, then there is a bit of a conflict because versionize now has 2 ways of generating a route name, alias or prefix_format. Would it generate both or would alias always have priority?

Another possible option is to just allow date objects to be major/minor versions, in addition to ints. This is probably the simpler solution.

ndejong commented 1 year ago

If you take the date-object approach, you might want to prevent timezone-aware date objects else you'd end up in a painful world of timezone confusion.

Perhaps the even easier middle ground is to use major versions in the format YYYYMMDD and then express the prefix format as

prefix_format = "/{major[0:4]}-{major[4:6]}-{major[6:8]}"

This would almost get us there; I suspect the "problem" then is with some of the generated documentation and /versions route?

alexschimpf commented 1 year ago

I don't really see a benefit in that approach vs. just using "YYYY-MM-DD" as your major version and using prefix_format as "{major}". Both "YYYY-MM-DD" and "YYYYMMDD" should sort just fine, so I don't see it being a problem. However, that prefix format would not work in the current implementation, which just uses simple string.format(...) to generate the prefixes.

My real concern is just allowing any arbitrary string, since the user must ensure that their version format will sort properly.

For example, if someone had versions like "2022-10-10" and "2022-7-10", versionize would incorrectly think "2022-10-10" came before "2022-7-10", which was not intended. That will cause problems.

ndejong commented 1 year ago

Yes, breaking the strictness of the int irks me too.

How about a versionize() flag that allows the developer-user to optionally enable "dangerous" string versions.

This way the default good behavior of versionize does not change, while it still gives the developer a way to accept and acknowledge they know what they are stepping into if they do. Perhaps an attribute versions_as_strings: bool

An alternative approach might be to introduce a new versionize_by_string() method that would allow versionize() to keep it's strict int args and again open up the possibility for the above.

Anyway, I'm sure there are other oddball situations where non-integer based versions probably make sense (although less common) so it's a feature/extension that is more than just accommodating a couple of silly dash characters in a date-looking-format string.

kunesj commented 1 year ago

I think, that it should be theoretically possible to use any hashable object as a version, if version_format and prefix_format could be functions.

That will also add support for more complex prefixes and versions. For example, I always want the last version to have "/dev" prefix, and I have to hack that right now.

The sorting problem could be resolved by forcing the developer to provide a function that will be used as a key in sorted(), if they want to use "unsafe" versions (not int).

alexschimpf commented 1 year ago

Ok so maybe something like this:

  1. Change api_version and api_version_remove to allow for major/minor to be anything hashable
  2. Add a new param to versionize called version_sort. version_sort would be a function used to sort the versions however you want (passed as key to sorted). If not provided, the default sort order of sorted is used.
  3. Perhaps you could also provide a list of optional aliases for each version, too. So like api_version(1, 3, aliases=['alias1', alias2']).

Does this all sound about right?

@kunesj Does the enable_latest and latest_prefix params not work for you regarding wanting the "last version to have "/dev" prefix"?

kunesj commented 1 year ago

@alexschimpf

Does this all sound about right?

That looks OK.

Also, I remembered that the versions have to be comparable (<, >, ...), otherwise the default sorting without a custom version_sort function will not work. But, I think that the TypeError that is raised when that happens is pretty clear, so I don't think that there really needs to be a check for that.

Does the enable_latest and latest_prefix params not work for you

latest_prefix just creates alias, but I want to completely replace the old prefix. If I have versions 1.0 and 2.0, I want to have only prefixes /v1 and /dev, because /v2 is not released yet, and I don't want anyone to accidentally use it without understanding that it can change.

alexschimpf commented 1 year ago

@kunesj @ndejong How does this look for a solution: https://github.com/alexschimpf/fastapi-versionizer/compare/alex/non-integer-versions?expand=1 ?

alexschimpf commented 1 year ago

@kunesj @ndejong I've created a new PR for this btw: https://github.com/alexschimpf/fastapi-versionizer/pull/18