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

remove /session/account from spec #14

Open
gr2m opened this issue Nov 28, 2015 · 8 comments
Open

remove /session/account from spec #14

gr2m opened this issue Nov 28, 2015 · 8 comments

Comments

@gr2m
Copy link
Member

gr2m commented Nov 28, 2015

Now that I'm implementing the account-json-api spec, I find the /session/account route confusing, because an account can exist and be accessible even without a session. For example, a user can signed up for an account but not sign in until the account is confirmed.

For that reason, I'd suggest to rename /session/account to /account, and /session/account/profile to /account/profile

Update

We now suggest to get rid of /session/account altogether, and use /accounts/{id} instead

@patriciagarcia
Copy link
Collaborator

Hey! /session/account was always a bit confusing to me, I was never sure if it was completely REST (although I got used to it), but then, if you rename it, shouldn't it better be to /accounts (POST /accounts, GET /accounts/id, etc)? It seems weird to me that there is an /accounts and an /account url.

@gr2m
Copy link
Member Author

gr2m commented Feb 22, 2016

Yeah good point ... First I was thinking: how about we prefix /acounts to make clear that this is for people with admin rights, to manage /admin/accounts? But from a SPEC perspective, your idea is much cleaner. And I think simplicity / clarity is a spec’s best friend :) I’ll let it sink, maybe someone else has an opinion here

@patriciagarcia
Copy link
Collaborator

Yeah, let's see what other people's opinion is, would be nice to hear from somebody that can look at it with a new perspective, we're too close to it to appreciate if something is too complex. Also if we're going to move from /session/account to /accounts there is a ton of things to change so better be sure before starting putting everything upside down :-)

@gr2m
Copy link
Member Author

gr2m commented Feb 24, 2016

to summarize this for people new to this spec. This is what we have today:

PUT    /session
GET    /session
DELETE /session
PUT    /session/account
GET    /session/account
PATCH  /session/account
DELETE /session/account
GET    /session/account/profile
PATCH  /session/account/profile
POST   /accounts
GET    /accounts
GET    /accounts/{id}
PATCH  /accounts/{id}
DELETE /accounts/{id}
GET    /accounts/{id}/profile
PATCH  /accounts/{id}/profile

This is what we would suggest instead

PUT    /session
GET    /session
DELETE /session
POST   /accounts
GET    /accounts
GET    /accounts/{id}
PATCH  /accounts/{id}
DELETE /accounts/{id}
GET    /accounts/{id}/profile
PATCH  /accounts/{id}/profile

For sign up, a new user would send POST /accounts instead of PUT /session/account. And a user would need to know their id. To fetch their account they would do GET /accounts/{id} instead of GET /session/account

I like the change, I think it makes things simpler to understand. I think with the current spec we tried to be smart, and I think it is smart, but that doesn’t make it simple.

@patriciagarcia
Copy link
Collaborator

Now that I see it like that, I like the second option much better, most of the questions we had with JSON API, where we had to guess the answers were because of the uncommon /session/account and /session/account/profile endpoints.

Just one thing, how do users know about their ids? I guess they'll get that information when requesting the session?

@gr2m
Copy link
Member Author

gr2m commented Feb 24, 2016

Just one thing, how do users know about their ids? I guess they'll get that information when requesting the session?

Yes. Or when creating an account. Optionally POST /accounts can send an id with the sign up request, which JSON API explicitly allows.

@rektide
Copy link

rektide commented Feb 24, 2016

I like the proposed new API!

I also have a healthy dollop of respect for the old /session/account endpoint. It's useful. While having a context (here: session) dependent resource is imo antithetical to the idea of a url (violates the u- universal), it was nicely namespaced in /session, which is a single sensible, container for this stuff.

Might it be possible to retain /session/account, and have it redirect to the canonical?

@gr2m
Copy link
Member Author

gr2m commented Feb 24, 2016

thanks for the feedback @rektide!

Maybe one way to think of this is that our Account JSON API Spec is like a minimal common dominator of implementations. If you decide to implement it, you can add own routes, as long as you remain compatible with the core spec. That we we could leave ideas like /session/account redirect to implementations, like https://github.com/hoodiehq/hoodie-account-server.

Would that make sense?

@gr2m gr2m changed the title change /session/account to /account remove /session/account from spec Feb 27, 2016
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

No branches or pull requests

3 participants