Open joezappie opened 2 years ago
So after a bit more discussion on Discord today and based on #2900 think that this can all be addressed now very flexibly with a $select
query resolver. For example, the below query resolver, excludes the user
by default unless explicitly selected (based on the original schema property names) and allows a $select: ['*']
for the standard properties:
const defaultSelect = Object.keys(messageSchema.properties).filter(key => key !== 'user')
export const loginQueryResolver = resolve<MessageQuery, HookContext>({
properties: {
$select: async (value) => {
if (Array.isArray(value)) {
return value.reduce((current, name) => [
...current, ...(name === '*' ? defaultSelect : [name])
], [])
}
return defaultSelect
}
}
})
@daffl I'm finally getting around to updating to the release and switching my current codebase to use all these new features. I didn't really put much thought into it before as your example looked good.
I would like to make an app wide hook to add in an $unselect
feature as that's a more common use case for me. 98% of the time I want to select everything, but sometimes when internally calling other services I want to exclude a resolver because it will result in a circular dependency.
My thought was to add a hook that converts $unselect
to the $select
notation by inverting it. The issue I'm having is, adding this as an app wide hook to add $unselect
for all services, I have no good way of dynamically getting the schema that will be used to grab the properties. I thought about trying to loop through context.self.hooks
but since resolveResult
and resolveQuery
return an anon function there's no good way to check if the hook contains a resolve
. I'd also have to call the function, to grab the first parameter which feels hacky. Curious if you have any ideas on this. My other option is to keep doing it the way I'm doing it now and write a wrapper to resolveResult/resolveQuery that I use instead, which is where I'll probably go with this.
A question I have, does $select
get passed to mongo for it to filter the document or does feathers filter it after getting the entire document?
I still think an $unselect
feature would be generally beneficial. If I submitted a PR to add it, would you consider it? Id rather spend time working on something others might also get use out of then something just for me.
Looking through the code, I see $select
does get passed to the db query. Thinking about this more, an $unselect
does pose an issue when it comes to SQL as it doesn't (at least the major ones) support column exclusion, only inclusion (though mongo and an in memory db could support it). It could work by doing the field removal in the final result, but that's probably not ideal.
Does the mongo adapter support the object syntax for $select
such as { _id: 0, name: 1}
directly? If resolve.ts
could support either format, then the mongo notation could be used to exclude a resolver from running instead of a separate $unselect keyword, and could be a mongo only feature. This would be a big change though I'm guessing?
That all said, its my fault for not looking at this closer in December to give feedback when you were going through. I'm totally okay with just writing my own wrapper around resolveResult/resolveQuery, just wanted to put in my 2 cents even if its to late at this point. This issue could be closed now that property filtering is now doable.
I believe this would still be possible, although it'd have to be on the schema/service level since we need to know about all the properties (easier introspection is definitely something I've been thinking about though). Based on my above example, a $unselect
might look like this:
const properties = Object.keys(messageSchema.properties)
export const loginQueryResolver = resolve<MessageQuery, HookContext>({
$unselect: async () => undefined,
$select: async (value, data) => properties.filter(prop => !(data.$unselect || []).includes(prop))
})
You're right, better to do that as a resolver than making a wrapper for resolveResult
/resolveQuery
. I can still turn that into a service hook I call before resolveResult
and pass it the schema there. My main concern was that it must be dealt with on the service level, instead of being able to add new global functionality. But since you're thinking about ways of making introspection easier, I'm good to go forward with you're resolver suggestion and consider that resolved.
Edit
That said, the only other hang up I have is virtual properties with $select. Since querySyntax
takes in the schema and doesn't know about the resolvers, ajv fails to validate when selecting a virtual property. Would be simple to make a conversion function that takes the resolver and spits out a schema with Type.Any
for them just for generating the querySyntax
, but do you have any thoughts on this? Maybe querySyntax
should be able to take in an optional resolver as well?
const schema = Type.Object({
_id: Type.ObjectId(),
employeeId: Type.ObjectId(),
});
const resolver = resolve({
employee: virtual((object, context) => {
return context.params.loader.service('api/org/employees').load(object.employeeId);
}),
});
export const querySchema = Type.Intersect(
[
querySyntax(schema),
Type.Object({}, { additionalProperties: false }),
],
{ additionalProperties: false }
);
Took me a bit to realize this was what's going on.
This is in relation to #2595 but I'm making my own thread as this detours from their original request.
The proposed solution is a built in filter list for resolvers. There is one mention in the dove docs for a $resolve property, but no information on it so I'm assuming its up to developers to roll their own at this point. I think this is a common/generic enough feature that should be built into the schema API.
Proposal
I'm proposing two query parameters (though the names probably arnt the best):
These can either be an array of property names, or an object that includes what type of resolver is being run (data/query/result) and its array of properties. This is needed because you may have a query resolver with a result resolver and in that case you need to specify what resolver its for.
Querying Params
Both $resolve and $exclude can be provided. Exclude is done as a 2nd pass to modify the selected properties, so in this case it would just further filter down the $resovle list.
Explicit inclusion I also included a helper function hasProperty(). This is useful for if you want to specify a property that should only run if its explicitly listed. Instead of having to list a bunch of resolve properties or specifically exclude the property property, this by default allows that. I'm using something similar where my frontend generally needs less information, but if I'm doing a backend service call I might want more details on top of the typical resolvers.
Issues
While this is completely doable in userland, theres two things I don't like about my current solution.
find: [(context) => resolveResult(filterProperties(resolve({...}))(context))(context)],
resolveResult(filterProperties(resolve({...}), 'result')(context))(context)
Currently Implementation
Tested exclude/include but not for running based on type of resolver.
Note: I did include the resolveFilteredResult hook that wraps the resolveResult method. That does make my original point about it being messy less relevant but I still feel like this is a common enough thing to be warranted. If you're not using it, performance wise it would have a negligible impact.