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

feat!: expose type of selection via openapi #1050

Closed
wants to merge 2 commits into from
Closed

Conversation

brprice
Copy link
Contributor

@brprice brprice commented May 30, 2023

No description provided.

@brprice brprice requested a review from a team June 7, 2023 07:18
@brprice brprice marked this pull request as ready for review June 7, 2023 07:18
@@ -98,6 +98,13 @@ data SessionAPI mode = SessionAPI
:> Get '[JSON] Prog
, getSessionName :: GetSessionName mode
, setSessionName :: SetSessionName mode
setSelection ::
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bikeshedding: we could common this up with the "get available actions" api or split into a "set selection" and "get current node's type"

Copy link
Contributor

Choose a reason for hiding this comment

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

we could common this up with the "get available actions" api

I don't really see how that would work?

or split into a "set selection" and "get current node's type"

This is more like what I would have expected. Although I don't know if there are strong arguments either way, and I'm happy to revisit it later. I was actually expecting that we'd instead take the selection as an argument, rather than doing anything stateful at all. We have previously considered keeping track of all selections in the backend and decided against it: #706.

This adds a new endpoint to the OpenAPI, but does not change any
existing endpoints.

Signed-off-by: Ben Price <[email protected]>
@dhess
Copy link
Member

dhess commented Jun 7, 2023

As currently implemented, this PR fails OpenAPI schema checks (sometimes, as that check is Gen'ed and therefore isn't deterministic):

https://buildkite.com/hackworthltd/primer/builds/3523#0188956b-19f8-4758-8f55-4670487c269c/17-31

The root cause appears to be due to the fact that setSelection returns a Maybe TypeOrKind:

https://github.com/hackworthltd/primer/pull/1050/files#diff-a52d39dcbeec647f54334d5449e78208c0f7e9cc1cbb60f8e45ca5382c34b186R1226

and from what I gather, OpenAPI 3.0 doesn't handle this well. Here are some potentially relevant links, including a seemingly related issue upstream on our OpenAPI lib:

According to the upstream issue, we might be able to set omitNothingFields in our Aeson encoding, but it's not clear. I'm not sure what knock-on effects that might have, in any case. (It might actually reduce our database encoding sizes, I suppose?)

Should setSelection be returning plain TypeOrKind, rather than wrapping it with a Maybe? Is it possible to set a (valid) selection that doesn't have a type or kind? (edit: from @brprice comment in chat, "They should but technical reasons: we only have a Maybe TypeCache")

@dhess
Copy link
Member

dhess commented Jun 7, 2023

I've worked around the issue by converting Nothing return results to API errors. As I understand it, in practice, this should never happen unless something is wrong in our implementation, so this seems like probably the right thing to do anyway.

This fixes an issue where the OpenAPI schema check failed when
`setSelection` returns `Nothing`. This should never happen in theory
and is just a quirk of our typechecker implementation, but our OpenAPI
adapter can't properly handle this scenario, and the OpenAPI 3.0 spec
isn't clear how to do so, either.

To work around this, we add a wrapped version of `setSelection` that
converts `Nothing` results to an API error.

Signed-off-by: Drew Hess <[email protected]>
@dhess
Copy link
Member

dhess commented Jun 7, 2023

Closing in favor of #1058

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.

3 participants