Skip to content

feat: add relation manipulation to api, and update get routes to return relations as well#802

Merged
tefkah merged 25 commits into
mainfrom
tfk/api-relations
Nov 26, 2024
Merged

feat: add relation manipulation to api, and update get routes to return relations as well#802
tefkah merged 25 commits into
mainfrom
tfk/api-relations

Conversation

@tefkah
Copy link
Copy Markdown
Member

@tefkah tefkah commented Nov 21, 2024

  • feat: add rough sketch for new pub api routes
  • feat: add basic way to update and delete pub
  • fix: refine updating and removing slightly such that they work
  • feat: make upsert relation work sort of
  • chore: small cleanup
  • fix: move processedpubtype to contracts, replace output schema with processedpubschema for all pub operations
  • refactor: do not make update functions check for pub existence, move resp to caller
  • refactor: make communityId mandatory for getPubsWithRelatedValuesAndChildren
  • fix: return full pub from relation routes
  • chore: add descriptions to existing relation endpoints
  • refactor: change the output type of createPubRecursiveNew to be ProcessedPub, to match what is returned by getPubWithChildren etc
  • chore: clean up some function args
  • feat: add prefer header
  • feat: make it possible to filter pubs by stage and pubtype, and add correct query parameters to get pub endpoints
  • fix: make withChildren work properly
  • feat: allow generation of custom api access token for easier testing
  • feat: allow api to filter response of top level pubs to certain fieldslugs

Issue(s) Resolved

Resolves #738. I discussed this with @allisonking, as she probably did not have enough time to get to it this sprint, and it would be a small thing to add for me.
Resolves #739

High-level Explanation of PR

[!NOTE] Open qs

  • Should we return all children and relations by default? It does so currently, but that can get quite hairy, especially bc they cannot be paginated yet.

This PR adds endpoints for

  • updatePub at PATCH /pubs/<pubId>
  • removePub at DELETE /pubs/<pubId>
  • upsertPubRelations at PATCH /pubs/<pubId>/relations
  • replacePubRelations at PUT /pubs/<pubId>/relations
  • removePubRelations and removeAllPubRelationsBySlugs at DELETE /pubs/<pubId>/relations

It also updates the old GET /pubs and GET /pubs/<pubId> endpoints to use the new getPubsWithchildrenAndRelations function, and to accept more query parameters.

Additionally, it:

  • Adds support for the Prefer header on update endpoints. Setting Prefer: return=minimal will not return anything for PUT/PATCH/POST routes. This makes the request somewhat faster and saves bandwith. (i just learned about this and thought it would be neat to implement, can be removed)

I also fixed/changed some things for getPubsWithRelationsAndChildren:

  • withChildren and withRelatedPubs now work properly (they were inversed, ie withRelatedPubs: false would disable fetching of children, and vice versa)
  • filtering by fieldSlugs only applies to the top level pub now. I think this is more realistic behavior. We should probably implement more sophisticated filtering at some point, maybe look at how eg Strapi handles that
  • You can now filter by both stage and pubType, previously it would only be one or the other
  • You now have to provide a communityId. this does mean you cannot look at other communities' pubs anymore, but I think this is largely intended behavior. We should revisit this once we want cross-community communication.

Misc:

  • I added a way to generate api access tokens to the seed script. This makes it easier to test out the api locally, as you don't have to generate a token yourself every single time.
  • I changed the output of createPubRecursiveNew to match getPubsWithChildrenAndRelatedPubs.

[!WARNING] Breaking changes
The get endpoints now return the values as an array instead of an object! This is mostly to make my life slightly easier.
I checkd with @isTravis about this, but @gabestein you might also rely on this being the case.

I do think this is important and we should make a firm decision here about what this should be in the future.

Test Plan

This will not be fun, apologies. For now I think it's most important that fetching pubs works, and updating them and their relations. Whether super specific query parameters work as intended is less important (@isTravis please feel free to disagree!).

  1. pnpm reset. then pnpm dev. You can now use xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000 as a token for arcadia-research
  2. Try at least these requests (tip: add | jq at the end to get a nicer view of the result (do brew install jq if you don't have it yet))
  • Get all pubs (check that at most 10 pubs are returned)
curl 'http://localhost:3000/api/v0/c/arcadia-research/site/pubs' --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000'
  1. Try to find the Journal Article pub by finding its id through the UI, or by first finding the id of the journal article pubtype like something like this (yes, would be good to be able to search by name through the ui) (even better would be to give pub types slugs)
curl  'http://localhost:3000/api/v0/c/arcadia-research/site/pub-types?limit=1000'  --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000' \
        | jq '.[] | select(.name == "Journal Article") | .id'

You could then find the journal article pub like

curl  'http://localhost:3000/api/v0/c/arcadia-research/site/pubs?pubTypeId=<pubTypeId>'  --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000' | jq '.[0].id'

Finally, get the journal article pub.

curl 'http://localhost:3000/api/v0/c/arcadia-research/site/pubs/<pubId>' --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000'

Check that only one pub is returned, dand that is has a lot of children and relations.
It should have some eg a tag

  1. Try to add a tag to the journal article through the api
    a. First find the Tag pubtypes id. you can do that like this with jq
curl \
         --url http://localhost:3000/api/v0/c/arcadia-research/site/pub-types \
         --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-0000000
00000' \
        | jq '.[] | select(.name == "Tag") | .id'

b. Then, using the id of the journal article pub above, try to create a new related pub. (technically you can do this for any pub ofc)

curl --request PATCH \
  --url http://localhost:3000/api/v0/c/arcadia-research/site/pubs/<journal article pub id>/relations \
  --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000' \
  --header 'Prefer: return=representation' \
  --data '{
  "arcadia-research:tag": [{ "value": null, "relatedPub": {
    "pubTypeId": "35d468eb-169c-40eb-afb0-6ecacce156b4",
    "values":{"arcadia-research:title": "Wow, a new tag"}
  } }]
}'

Then, check the number of tags returned through the api (we don't have a way to view relationships in the ui yet)

curl --url 'http://localhost:3000/api/v0/c/arcadia-research/site/pubs/<journal article pub id>?fieldSlugs=arcadia-research:tag' --header  'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000' | jq

You shuold see at least two tags! Go to the /pubs page, you should see two Tag pubs.

  1. try the same as above, but with --request PUT instead. This will replace all the tags instead (it won't delete the tags though, just their relations to this pub)
curl --request PUT \
  --url http://localhost:3000/api/v0/c/arcadia-research/site/pubs/<journal article pub id>/relations \
  --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000' \
  --header 'Prefer: return=representation' \
  --data '{
  "arcadia-research:tag": [{ "value": null, "relatedPub": {
    "pubTypeId": "35d468eb-169c-40eb-afb0-6ecacce156b4",
    "values":{"arcadia-research:title": "Wow, a new tag"}
  } }]
}'

if you now GET the pub again, you shuold see there's only one tag!

  1. You should be able to just delete the tags
curl --request DELETE \
         --url http://localhost:3000/api/v0/c/arcadia-research/site/pubs/d460d438-4ed4-49ca-aea9-9bf2c52ef0ec/relations \
         --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000' \
         --header 'Prefer: return=representation' #this makes sure we get a full thing returned \
         --header 'user-agent: Keyrunner' \
         --data '{
     "arcadia-research:tag": "*" 
   }' | jq

in the return value you should not be able to see any arcadia-research:tag values anymore.

Screenshots (if applicable)

Notes

query: getPubQuerySchema.extend({
pubTypeId: pubTypesIdSchema.optional().describe("Filter by pub type ID."),
stageId: stagesIdSchema.optional().describe("Filter by stage ID."),
limit: z.number().default(10).optional(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm I get different results for

curl 'http://localhost:3000/api/v0/c/arcadia-research/site/pubs' --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000' | jq 'length'

18
curl 'http://localhost:3000/api/v0/c/arcadia-research/site/pubs?limit=10' --header 'Authorization: Bearer xxxxxxxxxxxxxxxx.00000000-0000-0000-0000-000000000000' | jq 'length'

10

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think limit can't be optional if we want the default to kick in (same for offset, I suppose, but that probably matters less since undefined also is 0)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah great catch!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i've fixed this!

Copy link
Copy Markdown
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

went through the very thorough testing instructions (thanks for writing all those curl commands out!!!) and everything worked as expected! except for the limit thing, but other than that I think it looks 💯

* fix: ___________ INIT for easy cherry picking later because i somehow still don't know how to rebase

* feat: add capabilities based auth to site api

* chore: change some error codes to be more accurate
Copy link
Copy Markdown
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

🎉

@tefkah tefkah merged commit 57bd15a into main Nov 26, 2024
@tefkah tefkah deleted the tfk/api-relations branch November 26, 2024 17:21
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.

Add ability to update and delete pub field values, including relationships, to API Add pub relationships to pub read api

2 participants