Skip to content

Add Sessions API controller implementation#8089

Merged
aduth merged 3 commits intomainfrom
aduth-sessions-api
Mar 30, 2023
Merged

Add Sessions API controller implementation#8089
aduth merged 3 commits intomainfrom
aduth-sessions-api

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Mar 28, 2023

🛠 Summary of changes

Extracted from #7966

This adds a new API controller for managing the user session status (retrieval, updating, and destroy). See #7966 for more context and rationale.

The reason for creating a separate pull request is to have this endpoint in place in a deployment prior to the one including JavaScript changes using the new endpoint (see related discussion).

📜 Testing Plan

  • rspec spec/controllers/api/sessions_controller_spec.rb
  • $ curl -H 'Content-Type: application/json' http://localhost:3000/api/internal/sessions
    • {"live":false,"timeout":"2023-03-28T20:24:49.584Z"}

aduth added 2 commits March 28, 2023 16:19
Backward-compatibility, works just as well
@aduth aduth changed the title Add Sessions API controller implementatioin Add Sessions API controller implementation Mar 28, 2023
config/routes.rb Outdated
Comment on lines +15 to +17
get '/api/sessions' => 'api/sessions#show'
put '/api/sessions' => 'api/sessions#update'
delete '/api/sessions' => 'api/sessions#destroy'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an internal API, like only for our own JS. WDYT about prefixing with /js or /api/js to clarify that it's different from all the endpoints above, which are for external clients? Maybe add a section comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting thought. We have a lot of other endpoints which are internal in the same way (examples). Personally, I hadn't considered there being a distinction between internal and public-facing API, at least as reflected in the route paths. I'm not opposed to it, but I'd be curious about naming and how we work to apply the convention consistently. And I suppose we'd probably want the class namespace to reflect it as well.

Maybe something like /api/internal/sessions / Api::Internal::SessionsController ?

I guess I worry that tying it to JavaScript may be too constraining, though I'm not really sure what other use-case there'd be where we'd expect to be calling these APIs, since the underlying distinction is that these endpoints are stateful and assume to be called with the session cookie.

Another idea could be to flip the expectation, and expect that a public API would be versioned, and an internal API would be any route without a version identifier, or at least a version identifier which marks it as unstable (e.g. v0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a namespacing of 'internal' in 80fd09b

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah I forgot about those because we alphabetize them lower 🙈

I like the /api/internal! seems like a thing we could consider standardizing on

@aduth aduth merged commit 4c8041e into main Mar 30, 2023
@aduth aduth deleted the aduth-sessions-api branch March 30, 2023 13:22
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.

2 participants