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

Refactor HTTP api to better adhere to REST guidelines #2225

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

coolreader18
Copy link
Collaborator

Description of Changes

Based off #2181. This mainly hits /database, making the endpoints be /database/:dbname/verb rather than /database/verb/:dbname, and making the http methods be more accurate to what they do (e.g. DELETE /database/:dbname rather than POST /database/delete/:dbname). Dns stuff like register_tld (actually, just register_tld for now) got factored into its own /domain path. Also, I put everything behind a /v1 prefix -- I could revert that if we don't wanna do that, but that feels like best practice for HTTP APIs in general.

API and ABI breaking changes

Yes.

Expected complexity level and risk

3: obviously, there's a lot of stuff moving around here, but the smoketests are pretty thorough in exercising all the routes of the API, and if there's anything I miss here it's pretty easy to change.

Testing

  • Smoketests and sdk tests exercising the api interactions in this repository.

@coolreader18 coolreader18 added the api-break A PR that makes an API breaking change label Feb 7, 2025
@gefjon
Copy link
Contributor

gefjon commented Feb 7, 2025

Needs companion PRs in private, TypeScript SDK, C# SDK and docs. I can wait on the docs one, but the first three must happen before we merge this.

@coolreader18
Copy link
Collaborator Author

Yep, makes sense 👍

@gefjon
Copy link
Contributor

gefjon commented Feb 7, 2025

That said, the changes look good, and I'll be happy to approve once we have the companion PRs ready to go.

@coolreader18
Copy link
Collaborator Author

@cloutiertyler
Copy link
Contributor

Also, I put everything behind a /v1 prefix -- I could revert that if we don't wanna do that, but that feels like best practice for HTTP APIs in general.

We definitely want to do that. I was meaning to do it for a long time.

Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. I have two comments/requests:

  1. In the Identity proposal and remove Address proposal, I proposed to remove the concept of TLD and domains in general, and instead replace it with database names, where a database name matches ^[a-z0-9]+(-[a-z0-9]+)*$ (lowercase alphanumeric with internal dashes). That leaves room in the future for having multipart names like foo.bar or foo/bar in the future. This is only relevant to this PR insofar as we may not want to introduce a new /domain route if we're going to just basically have /database/:name_or_identity/set-name route. That being said, we have little time left and deprecating the /domain route in favor of the new set-name route is not a big deal. I am also fine to have a fix for that happen in another PR.

  2. This needs a corresponding Private PR.

@coolreader18
Copy link
Collaborator Author

This is only relevant to this PR insofar as we may not want to introduce a new /domain route if we're going to just basically have /database/:name_or_identity/set-name route.

The actual only functionality there is "register_tld". Is that something that just doesn't need to exist anymore? I wasn't sure whether that was being fully removed or not; I figured it can just be merged into publish, perhaps.

@coolreader18 coolreader18 changed the base branch from master to noa/proper-schema-api February 10, 2025 17:55
@gefjon
Copy link
Contributor

gefjon commented Feb 10, 2025

This is only relevant to this PR insofar as we may not want to introduce a new /domain route if we're going to just basically have /database/:name_or_identity/set-name route.

The actual only functionality there is "register_tld". Is that something that just doesn't need to exist anymore? I wasn't sure whether that was being fully removed or not; I figured it can just be merged into publish, perhaps.

TLDs are not functional, and this PR has not caused them to become functional. Notably, using /es as separators within URL components is incompatible with Axum's routing (except in the specific case of being in trailing position, I think, which our database names aren't). Possible futures involve us revisiting TLDs with a different separator character, but we can do that in the future in a backwards-compatible way.

If the /domain route's only functionality is register_tld, then /domain should be removed.

@coolreader18 coolreader18 force-pushed the noa/http-glowup branch 2 times, most recently from 674d5d5 to b7d7dc9 Compare February 10, 2025 20:38
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further reflection I think that POST /database/:db/names is the correct route.

This now LGTM. I'm okay to merge this.

@coolreader18 coolreader18 merged commit 8a65885 into noa/proper-schema-api Feb 11, 2025
8 of 9 checks passed
@coolreader18
Copy link
Collaborator Author

Merged into the wrong branch 😔 #2243 is the PR into master.

bfops added a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Feb 11, 2025
## Description of Changes
Companion PR for the http api glowup.

## API

Not a breaking change; this catches us up to being compatible with a
breakage introduced by
- clockworklabs/SpacetimeDB#2225.

## Requires SpacetimeDB PRs
- clockworklabs/SpacetimeDB#2225

## Testsuite
SpacetimeDB branch name: master

## Testing
Existing CI passes (it was failing without this change since it couldn't
connect).

Co-authored-by: Zeke Foppa <bfops@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break A PR that makes an API breaking change release-1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants