feat(modules-sdk): remote query retrieve

[carlos-r-l-rodrigues]

What:

Remote Joiner options to check if keys exist on entry points or relations

[changeset-bot]

🦋 Changeset detected

Latest commit: d421a83

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@medusajs/modules-sdk	Patch
@medusajs/orchestration	Patch
@medusajs/types	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
medusa-dashboard	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 29, 2024 9:10am

3 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
api-reference	⬜️ Ignored (Inspect)			Mar 29, 2024 9:10am
docs-ui	⬜️ Ignored (Inspect)	Visit Preview		Mar 29, 2024 9:10am
medusa-docs	⬜️ Ignored (Inspect)	Visit Preview		Mar 29, 2024 9:10am

[sradevski]

I think this is fine, but I do have a question.

Why don't we add list and retrieve on remoteQuery? For example you'd do remoteQuery.list and remoteQuery.retrieve, and like that you will either get an array or a single object. Also the arguments we pass can be slightly different, eg. for retrieve you will only be able to pass an ID for filtering.

Again, not something we need to do now, but I am curious why we haven't opted for that approach

[carlos-r-l-rodrigues]

Again, not something we need to do now, but I am curious why we haven't opted for that approach

The Remote Joiner is a generic data fetching system that resolves relations always in bulk.
The Remote Query is built on top of it to call our modules and it is just being actively used now.

Why don't we add list and retrieve on remoteQuery? For example you'd do remoteQuery.list and remoteQuery.retrieve, and like that you will either get an array or a single object.

We can implement that. but I believe this should also come when we somehow can type the expected result of the query.

[olivermrbl]

Just chatted with @carlos-r-l-rodrigues about an alternative approach that would solve two common scenarios:

1. Validate that a single resource exist (the idea with this PR)
2. Validate that all resources exist (e.g. before creating links)

We could introduce an option to remoteQuery, something like { throwIfNotExist: true }. The remoteQuery would always use list - instead of retrieve as in this PR - but then throw if any of the passed IDs do not exist.

For both 1. and 2., it would look like:

const result = await remoteQuery({...}, { throwIfNotExist: true })

This would allow us to create generic steps for validating that resources exist instead of having steps for each domain like the following one:
https://github.com/medusajs/medusa/blob/develop/packages/core-flows/src/definition/cart/steps/validate-variants-existence.ts

What do you guys think? @sradevski, @adrien2p

[sradevski]

@olivermrbl

We could introduce an option to remoteQuery, something like { throwIfNotExist: true }. The remoteQuery would always use list - instead of retrieve as in this PR - but then throw if any of the passed IDs do not exist.

I think the only caveat here is that we will need to apply that rule only if you are filtering by ID (or in fact, primary keys), otherwise it wouldn't make much sense to have such a setting.

An alternative approach is to be explicit about what sort of data fetch you are trying to do - A retrieve that takes in id: string | string[] or a list with filters that can be anything. I think then you don't need the config as it will be implicitly clear that if an ID you passed to retrieve doesn't exist, it should throw (or we could still keep the setting if you are ok with partial results)

[adrien2p]

I am not sure it is the remote query responsibility to do this, already, if we call the retrieve from the modules, it will throw if no results are found for the given primary key. Also, it would only work for primary keys, which in my opinion could lead to miss understanding, e.g filtering using any filters + true for the throw option but there is no way to validate that internally. Shouldn't this be something done outside of the remote query. Like we usually do, find -> check -> throw?

Maybe we could have a validator predicate? e.g

const result = await remoteQuery({
  filters: {
    code: 'test'
  }
}, {
  throwOnNotFound: 'code'
})

const result = await remoteQuery({
  filters: {
    id: ['test']
  }
}, {
  throwOnNotFound: 'id'
})

above, it is explicit the field you want to validate the data against and if there is multiple filters it is also explicit that you can't do it and you have to handle it manually 🤔 I am totally unsure about this approach, just throwing it out loud ahah

[olivermrbl]

The idea was based on the assumption that we always use remoteQuery to fetch resources – at least in API routes and workflows. WIth that in mind, I thought would be nice to have some additional functionality baked into the remoteQuery, that you would otherwise have gotten from using the modules directly e.g. the existence check in retrieve.

[carlos-r-l-rodrigues]

I think it is fine to have this validation on the Remote Joiner. But the only validation it will be able to perform is if primary keys were returned, anything outside this scope is business logic and should be handled where it belongs.

So, the remote joiner will have options to throw in case the key is not found. Under the hood, the remote query will always call the method list, that will enable to easily have helpers on the remote query like .list or .retrive to make the return an array or not, and to validate the input with one or more keys.

[olivermrbl]

Right now, I believe we are in fact not handling the case of a non-existing resource in any of our GET /admin/[resource]/:id endpoints, because we are just responding with whatever remoteQuery spits back - which in this case is nothing.

Here, we would either have to add:

if (!remoteQueryResult) {
  throw 404
}

or use the option proposed here.

I am fine with either.

changes

[olivermrbl]

Given that this doesn't really change the behavior or anything in remoteQuery, I'd be down for merging this now. We can play around with it in API routes and then come to a decision whether or not to keep it at a later point?

Personally, I think this is a nice addition.