Open begedin opened 9 years ago
@begedin I think fetching a single users from "/users" wouldnt make much sense, thats not how REST is defined, but I think something like "/users/me" or "users/current" makes more sense considering "me" or "current" as an ID.
@idelahoz Yeah, that sounds like a good idea.
@begedin I think this is a case of incidental duplication. The current user diverges enough from another user on the system enough that we should be careful of treating them too much the same. That was the reasoning for making /me a separate call. Maybe /me
isn't the right name for it though.
How about this... look up how some well designed APIs on the internet do it and how other people think about the current user and REST. Maybe we'll get some ideas from there.
Did some research, here are my findings:
/me
within the users controller.show_current
within the users controller. GET 'users/me'
is mapped to the show_current
actionstaff#current_member
. However, it does something similar to what we do - matches GET 'current_member'
with staff#current_member
so it's about the same as we have it./me
endpoint at all.Maybe we do not need a /me
endpoint at all. The login response is already sending exactly what we need, so why not just use that? On the ember side, we simply take the response and push it to the store as current-user
. We could override the session hook that usually does it.
I think we should be re-authenticating any time we change the current-user
anyway, so there should not be a reason to fetch it explicitly. We sould just store what a successful login sends us.
Worst case, maybe move the endpoint for GET '/me'
to GET 'users/tokens/me'
. I don't really like that idea, though.
The thing that irks me most about the current state is that I have to do special case handling for the current-user adapter and serializer.
typeForRoot
method, that simply returns 'currentUser'
because the API returns a user record, so the serializer would serialize it as a user by default if I didn't override it. It may cause complications in the future.buildURL
method, which behaves completely differently from the usual buildURL
method. Usually, it accepts a typeName
, id
and snapshot
and builds from that. Ours accepts a path
because the path is different based on what we're actually doing. Our custom find
method calls buildURL
with a path of 'me'
, while our createRecord
method calls it with a path of users
. It works, but it's unconventional, so it might cause confusion.Ideally, we'd have a situation where no customization is required (so either we switch all to users, or the api has a CurrentUser
model as well. That's why my suggestion was to have a User
and a UserProfile
and to move some of the endpoints around. A split between User
and UserProfile
seems less confusing to me than User
and CurrentUser
. User
would be the private data that rarely changes for any user. We wouldn't really fetch it from the client for any user other than our own. UserProfile
would be the public data. We could change our own easily and we could see everyone elses depending on the scenario.
Less ideally, we'd have a situation where we access the same endpoint for get
and post
, (by having is similar to how https://github.com/paulmillr/ostio-api/ listed above has it - a get '/users/me'
. That way, at least the adapter custiomization would be lessened.
At the core of it, I'd like to match up our entities. If the API sends a User
record, then the client should be expecting a User
record, or vice versa, if the client is expecting a User
record, the API should send it. Currently, the client expects a CurrentUser
record, but the API sends a User
.
Additionally, I'd like to match up the endpoints, so a single entity on the client is accessing the same basic endpoint on the server for all actions on that entity (find, save, update, etc.).
Endpoints
At the moment, on the API side we have the following endpoints:
POST /users
- to register/create a User record (on the ember side asCurrentUser
)GET /me
- to fetch the current user (User
record, serialized asCurrentUser
,CurrentUser
on ember side)POST /users/token
- to log in using an email and password. Managed by single login method on ember side, not related to any model adapter, returns an authentication token (for theCurrentUser
on ember side)Models and entities on the API side
User
model hasid
,email
,encrypted_password
,authentication_token
CurrentUserSerializer
which hasid
,email
andauthentication_token
.Models on the ember side
User
andCurrentUser
. I wrote it that way due to usingreissued-ember
as an example.User
is not really used for anything yet. I assumed it would have an attribute calledusername
and ahasMany
relationship withorganizations
CurrentUser
has the propertiesusername
,email
,authenticationToken
,password
andpasswordConfirmation
.username
is unused. I assumed we'd use it and forgot to remove itemail
andauthenticationToken
are what we need to store in our session in order to authorize requests.password
andpasswordConfirmation
are only needed initially, to register a new user. Once sent, they are never retrieved back from the APISuggestions
Eliminate
GET '/me'
Registration and fetching of the
CurrentUser
are handled by theCurrentUser
adapter. In normal cases, the buildURL method of the adapter handles the endpoint for the entity, and there's usually one primary endpoint, accepting different methods. In our case, we have two methods, one toGET
(/me
) and one toPOST
(/users
).It complicates things a bit, so **I would like to eliminate the
GET '/me'
endpoint and make itGET 'users'
.Reorganize our entities
Having a
CurrentUser
andUser
both of which are actually theUser
entity on the API also makes things a bit confusing. Because of that, I suggest organizing it differently.User
andUserProfile
User
will contain the important properties -email, encrypted_password, authentication_token
, maybeorganizations
UserProfile
will contain the properties the user tends to change more often - we don't have any yet, but these would probably beusername/display_name, first_name, last_name, photo...
GET '/me'
endpoint is gone)User
andUserProfile
, with the same purposeUser
additionally haspassword
andpasswordConfirmation
for registration/password reset purposes