Simplify and openapi-ify /api/program #180

brprice · 2021-10-20T10:43:18Z

Simplify the return types from /api/program, and port to openapi3.
We drastically simplify by returning a rose tree where each node is labeled with
an ID and a textual label, instead of a full AST.

The rationale is that a frontend is mostly concerned with rendering and does not
need to know about the complexity of the full AST. This initial iteration is obviously
oversimplified and is expected to expand in the future.

brprice · 2021-10-20T10:58:20Z

Whilst the code is not ready for review, I did want to raise two questions:

How stable do we want the "old api" endpoints to be? (Or, how strongly-typed do we want the api to be?) In particular, Name and ID serialise as {unID: 5}, but the FR specifies the plain (string/)number 5. (NB: SessionName serialises as a plain string, and the generated TypeScript bindings have them as plain strings. The IDs are considered objects with one unID:number field)
We want to do some data munging so the service exposes simplified trees, rather than our full AST. There is the option of doing this in the core package in the Primer.API module, or in the service package. I am leaning towards doing it in the core package and keeping the service package as a thin servant wrapper (similar to how pagination is dealt with - though there is not much choice if we want to paginate at the DB level!). However, I could see arguments for making it easy to expose a more complex AST for alternative frontends.

@dhess: thoughts?

dhess · 2021-10-20T12:27:00Z

Whilst the code is not ready for review, I did want to raise two questions:

How stable do we want the "old api" endpoints to be? (Or, how strongly-typed do we want the api to be?) In particular, Name and ID serialise as {unID: 5}, but the FR specifies the plain (string/)number 5. (NB: SessionName serialises as a plain string, and the generated TypeScript bindings have them as plain strings. The IDs are considered objects with one unID:number field)

The old API is irrelevant, so don't make any compromises on the new API to accommodate it. If the old API continues to work (for some definition of "work"), that's fine, but if it doesn't, we can just remove the code and/or Servant endpoint. (Over time the old API should go away entirely.)

We want to do some data munging so the service exposes simplified trees, rather than our full AST. There is the option of doing this in the core package in the Primer.API module, or in the service package. I am leaning towards doing it in the core package and keeping the service package as a thin servant wrapper (similar to how pagination is dealt with - though there is not much choice if we want to paginate at the DB level!). However, I could see arguments for making it easy to expose a more complex AST for alternative frontends.

Hmm, that's an interesting point that I hadn't considered re: alternate ASTs for, e.g., GHCJS-based frontends. (We might even be able to take advantage of that ourselves in some distant future.)

Thinking ahead, we could offer the more robust ASTs as a different API endpoint. Therefore, let's keep a 1:1 relationship between primer Haskell API and primer-servant HTTP API and do the tree-munging in primer.

brprice · 2021-10-20T14:01:53Z

This is now ready for review. My main question is over serialisation of IDs and Names. It is probably nicer if the api emits integers and strings instead of objects {"unID":5}. The choice we have is whether to wrap everything in unName etc, or to just change the serialisation of names.

primer/src/Primer/API.hs

primer/src/Primer/Name.hs

georgefst

Looks good, nothing major.

primer-service/src/Primer/OpenAPI.hs

primer/src/Primer/API.hs

primer/test/Tests/API.hs

primer/src/Primer/API.hs

brprice · 2021-11-04T18:16:53Z

@dhess : I believe you said that you wanted to review this PR.
I have addressed most of the bikeshedding etc above, except:
There are two things still to do

rename children to avoid react footguns
decide what to do with the _type field. I need to look into how to mangle field names in aeson/openapi

dhess

Only some minor comments & questions. Looks good to me, thanks!

primer-service/src/Primer/OpenAPI.hs

primer-service/src/Primer/Server.hs

primer/src/Primer/API.hs

dhess · 2021-11-10T16:26:33Z

primer/src/Primer/API.hs

+
+instance ToJSON Prog
+
+-- | This type is the api's view of a 'Def'


Doesn't this beg the question? :)

Thanks! This used to be called APIDef, but the comment bitrotted in the name change. I should say ... of a 'Primer.Core.Def', or whereever it is defined. I should also check the other api types, as I did a similar transition for something else also.

(There should be a way to a haddock lint pass, if there isn't already.)

dhess · 2021-11-10T16:29:16Z

primer/src/Primer/API.hs

+          <$> Map.toList (progDefs p)
+    }
+
+-- | A simple method to extract 'Tree's from 'Expr's. This is injective.


Is there a category theory-ish library on Hackage that we can use to wrap the Tree output type with an injective assertion/invariant?

primer/src/Primer/Core.hs

primer/test/fixtures/action.json

brprice · 2021-11-15T18:24:11Z

bors merge

ghost · 2021-11-15T18:24:27Z

Merge conflict.

brprice · 2021-11-15T19:06:05Z

Merge conflict.

Huh, I obviously don't understand bors. I had hoped that since a git merge on the command line completed successfully, that bors would be able to merge. Evidently bors' notion of "merge conflict" is stricter. I think I recall using bors to merge other PRs that github said had "conflicts that must be resolved" when the merge was trivial. I wonder what is different this time?

[Context: I didn't just rebase on main as the history of doing this PR before #192 is somewhat useful to understand why I added a bunch of unit tests. I guess I'll rebase tomorrow, to give me a chance to poke this PR to correct my misunderstanding]

dhess · 2021-11-15T19:24:57Z

Unfortunately, the Bors details page isn't very helpful: https://bors.hackworth-corp.com/batches/196

I'll try to look in the logs.

dhess · 2021-11-15T20:55:48Z

Unfortunately, the logs were useless.

brprice · 2021-11-16T12:07:31Z

I have rebased onto c81d082 and will test a bors merge. However, I expect this to still merge conflict.
bors try

ghost · 2021-11-16T12:07:34Z

try

Merge conflict.

This never fails. Let's expose that fact in the API.

i.e. we now have '1' and '"foo"' instead of '{unID:1}' and '{unName:"foo"}' Almost all of this commit is testcase churn because of this change.` The change to ToJSONKey enables us to output 'Map ID _' from our API (whilst having correct OpenAPI3 documentation), since the map will be serialized as an object, where the keys are 'show' of the IDs. However, we decide to keep our list-based representation, as documented in a comment. Note that this only applies to the "new api", and not to the serialization test fixtures.

We also add regression tests. [Historical note: this patch series was authored before Gen.Core.Raw obtained better coverage. This is the reason the non-injectivity was not discovered earlier, and the reason for the reliance on unit tests here.]

brprice · 2021-11-16T12:37:49Z

Rebased onto main. Bors should now be happy
bors merge

ghost · 2021-11-16T12:39:32Z

Build succeeded:

ci/hydra:hackworthltd:primer-bors-staging:required

dhess · 2021-11-16T12:43:46Z

Did you gain any insight on this as to why Bors rejected the earlier merge and the rebase on c81d082?

brprice · 2021-11-16T12:54:35Z

Did you gain any insight on this as to why Bors rejected the earlier merge and the rebase on c81d082?

No. I guess that what Bors considers a "merge conflict" is stricter than what (my local configuration of) git will automatically merge. However, I couldn't easily find any information about this, other than that Bors just calls the github api https://docs.github.com/en/rest/reference/repos#merge-a-branch

brprice linked an issue Oct 20, 2021 that may be closed by this pull request

Initial api for "getTree" #176

Closed

brprice force-pushed the brprice/getTree-0.1 branch from c5d134c to 477fcd8 Compare October 20, 2021 13:59

brprice commented Oct 20, 2021

View reviewed changes

primer/src/Primer/API.hs Outdated Show resolved Hide resolved

brprice commented Oct 20, 2021

View reviewed changes

primer/src/Primer/API.hs Outdated Show resolved Hide resolved

brprice commented Oct 20, 2021

View reviewed changes

primer/src/Primer/API.hs Outdated Show resolved Hide resolved

brprice force-pushed the brprice/getTree-0.1 branch from 776f221 to e597d96 Compare October 20, 2021 14:09

brprice commented Oct 20, 2021

View reviewed changes

primer/src/Primer/API.hs Outdated Show resolved Hide resolved

brprice commented Oct 20, 2021

View reviewed changes

primer/src/Primer/Name.hs Show resolved Hide resolved

brprice force-pushed the brprice/getTree-0.1 branch from e597d96 to c484db3 Compare October 20, 2021 14:33

brprice marked this pull request as ready for review October 20, 2021 14:33

brprice requested a review from a team October 20, 2021 14:33

brprice changed the title ~~Brprice/get tree 0.1~~ Simplify and openapi-ify /api/program Oct 20, 2021

georgefst approved these changes Oct 21, 2021

View reviewed changes

primer-service/src/Primer/OpenAPI.hs Outdated Show resolved Hide resolved

primer-service/src/Primer/OpenAPI.hs Show resolved Hide resolved

primer/src/Primer/API.hs Outdated Show resolved Hide resolved

primer/test/Tests/API.hs Show resolved Hide resolved

dhess mentioned this pull request Oct 21, 2021

Remove unnecessary Result type, replacing with isomorphic Either #183

Merged

brprice self-assigned this Oct 21, 2021

brprice requested a review from dhess October 21, 2021 14:43

brprice added the Do not merge Do not merge label Oct 26, 2021

brprice mentioned this pull request Oct 26, 2021

Document/improve "raw" generator #186

Closed

brprice commented Oct 26, 2021

View reviewed changes

primer/test/Tests/API.hs Outdated Show resolved Hide resolved

brprice commented Oct 26, 2021

View reviewed changes

primer/src/Primer/API.hs Outdated Show resolved Hide resolved

brprice mentioned this pull request Oct 28, 2021

feat(components): a simple HTML-based Tree component hackworthltd/primer-app#112

Merged

brprice force-pushed the brprice/getTree-0.1 branch from c484db3 to 2d0865b Compare November 4, 2021 18:10

brprice commented Nov 4, 2021

View reviewed changes

primer/src/Primer/API.hs Outdated Show resolved Hide resolved

dhess approved these changes Nov 10, 2021

View reviewed changes

brprice force-pushed the brprice/getTree-0.1 branch 2 times, most recently from 3f21582 to 4fde3fb Compare November 15, 2021 17:13

brprice force-pushed the brprice/getTree-0.1 branch from 4fde3fb to c485e6c Compare November 15, 2021 17:21

brprice removed the Do not merge Do not merge label Nov 15, 2021

brprice force-pushed the brprice/getTree-0.1 branch from c485e6c to 9284396 Compare November 16, 2021 12:05

brprice added 4 commits November 16, 2021 12:07

[primer] api: remove error wrapper on 'getProgram'

058b266

This never fails. Let's expose that fact in the API.

First iteration of tree api

a4f6e74

fix: make viewTree* injective

837f037

We also add regression tests. [Historical note: this patch series was authored before Gen.Core.Raw obtained better coverage. This is the reason the non-injectivity was not discovered earlier, and the reason for the reliance on unit tests here.]

brprice force-pushed the brprice/getTree-0.1 branch from 9284396 to 837f037 Compare November 16, 2021 12:25

ghost merged commit 4ea8eba into main Nov 16, 2021

ghost deleted the brprice/getTree-0.1 branch November 16, 2021 12:39

georgefst mentioned this pull request Jul 14, 2022

Render a module from the database hackworthltd/primer-app#426

Merged

dhess mentioned this pull request Jul 28, 2022

What to do about VJSON? #585

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and openapi-ify /api/program #180

Simplify and openapi-ify /api/program #180

brprice commented Oct 20, 2021 •

edited

Loading

brprice commented Oct 20, 2021

dhess commented Oct 20, 2021 •

edited

Loading

brprice commented Oct 20, 2021

georgefst left a comment

brprice commented Nov 4, 2021

dhess left a comment

dhess Nov 10, 2021

brprice Nov 11, 2021

dhess Nov 11, 2021

dhess Nov 10, 2021

brprice commented Nov 15, 2021

ghost commented Nov 15, 2021

brprice commented Nov 15, 2021

dhess commented Nov 15, 2021

dhess commented Nov 15, 2021

brprice commented Nov 16, 2021

ghost commented Nov 16, 2021

brprice commented Nov 16, 2021

ghost commented Nov 16, 2021

dhess commented Nov 16, 2021

brprice commented Nov 16, 2021 •

edited

Loading


		instance ToJSON Prog

		-- \| This type is the api's view of a 'Def'

Simplify and openapi-ify /api/program #180

Simplify and openapi-ify /api/program #180

Conversation

brprice commented Oct 20, 2021 • edited Loading

brprice commented Oct 20, 2021

dhess commented Oct 20, 2021 • edited Loading

brprice commented Oct 20, 2021

georgefst left a comment

Choose a reason for hiding this comment

brprice commented Nov 4, 2021

dhess left a comment

Choose a reason for hiding this comment

dhess Nov 10, 2021

Choose a reason for hiding this comment

brprice Nov 11, 2021

Choose a reason for hiding this comment

dhess Nov 11, 2021

Choose a reason for hiding this comment

dhess Nov 10, 2021

Choose a reason for hiding this comment

brprice commented Nov 15, 2021

ghost commented Nov 15, 2021

brprice commented Nov 15, 2021

dhess commented Nov 15, 2021

dhess commented Nov 15, 2021

brprice commented Nov 16, 2021

ghost commented Nov 16, 2021

try

brprice commented Nov 16, 2021

ghost commented Nov 16, 2021

dhess commented Nov 16, 2021

brprice commented Nov 16, 2021 • edited Loading

brprice commented Oct 20, 2021 •

edited

Loading

dhess commented Oct 20, 2021 •

edited

Loading

brprice commented Nov 16, 2021 •

edited

Loading