fix: Add type to path #870
Conversation
When query definition had a type, /data/io.cozy.triggers was fetched instead of '/jobs/triggers'.
| @@ -107,9 +107,9 @@ class TriggerCollection extends DocumentCollection { | |||
| * @throws {FetchError} | |||
| */ | |||
| async find(selector = {}, options = {}) { | |||
| if (Object.keys(selector).length === 1 && selector.worker) { | |||
| if (Object.keys(selector).length > 0 && selector.worker && selector.type) { | |||
There was a problem hiding this comment.
This change is a breaking change, and I'm not sure we really want to force the use of the type. In some case, it is wanted to not select on the type.
We should handle those cases:
- only worker
- only type
- both of them
WDYT?
There was a problem hiding this comment.
I agree. Moreover the stack implements a way to filter through multiple types (/jobs/triggers?Type=@Cron,@webhook). Since Mango already supports this via $in, I propose that we support:
Q('io.cozy.triggers').where({
type: {
$in: ['@cron', '@webhook']
}
})
Q('io.cozy.triggers').where({
type: '@cron'
})
Q('io.cozy.triggers').where({
type: '@webhook'
})
There was a problem hiding this comment.
cc @nono, are there other APIs from the stack that have the same filtering ability ? Would it make sense for those APIs to support the mango selector so that it is not done client-side ? It would make the client-side simpler.
There was a problem hiding this comment.
are there other APIs from the stack that have the same filtering ability ?
I think we can find 2 or 3 endpoints with filtering like that. Scanning the full docs should answer that.
Would it make sense for those APIs to support the mango selector so that it is not done client-side ?
Meh, I don't like that. It means creating a new endpoint with POST, and parsing the mango selectors, and making a response compatible with the expected format. And it is probably a good way to have silent errors (like putting a limit in the mango filter that will be ignored).
There was a problem hiding this comment.
I made changes taking into account your comments
| } else { | ||
| url += `Type=${type}` | ||
| } | ||
| } |
There was a problem hiding this comment.
Could the URLSearchParams API be used instead of building the params manually ?
https://developer.mozilla.org/fr/docs/Web/API/URLSearchParams
sp = new URLSearchParams()
URLSearchParams { }
sp.append('Worker', 'toto')
sp.toString()
"Worker=toto"
sp.append('Type', '@cron')
sp.toString()
"Worker=toto&Type=%40cron"
@nono Will the "%40" be correctly parsed by the stack ? I see that in the doc, the "@" is not url-encoded. cf https://github.com/cozy/cozy-stack/pull/2884/files
| const selectorLen = Object.keys(selector).length | ||
| const { worker, type } = selector | ||
| const hasOneKey = selectorLen === 1 | ||
| const hasWorkerAndType = worker && type && selectorLen === 2 |
There was a problem hiding this comment.
Mmh, I think we could simplify a bit the logic:
const { worker, type, ...rest } = selector
const hasOnlyWorkerAndType = Object.keys(rest).length === 0
if (hasOnlyWorkerAndType) {
// use the /jobs/triggers URL.
}
flohhhh
force-pushed
the
fix-trigger-collections
branch
3 times, most recently
from
ffe5c09 to
0c32776
Compare
flohhhh
force-pushed
the
fix-trigger-collections
branch
from
0c32776 to
f7d7d55
Compare
|
Thanks for the changes, LGTM, it seems that the PR is red because of linting issues. |
`/jobs/trigger`is called in some conditions: if there is only type or worker if there are type and worker
flohhhh
force-pushed
the
fix-trigger-collections
branch
from
f7d7d55 to
3ea6132
Compare
When query definition had a type, /data/io.cozy.triggers was fetched
instead of '/jobs/triggers'.