Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix item.read hook not firing for readByQuery #6645

Merged
merged 2 commits into from
Jul 7, 2021

Conversation

MoltenCoffee
Copy link
Contributor

@MoltenCoffee MoltenCoffee commented Jul 4, 2021

As reported by @lluishi93 in #6622 the current implementation of the item.read hook is flawed, as it doesn't get called with certain read actions.

I've moved the calling of the hook to the readByQuery method (which gets called by readOne and readMany), but there is still a problem. Normally, the hook gets called as follows:

emitAsyncSafe(`${this.eventScope}.eventType`, {
  event: `${this.eventScope}.eventType`,
  accountability: this.accountability,
  collection: this.collection,
  item: keys, // notice this
  action: 'eventType',
  payload: records,
  schema: this.schema,
  database: getDatabase(),
});

In readByQuery, however, the keys array is not available, as it consumes a query, already with the keys implemented.

I see several options:

  1. Make an exception and don't return item here

  2. Pass keys as an additional parameter to readByQuery; The significant drawback here is that readByQuery is called from a lot of places in the codebase, and I feel like this would provide some 'code smell'

  3. Retrieve from the query as follows:

    const primaryKeyField = this.schema.collections[this.collection].primary;
    
    const keys = query.filter?.[primaryKeyField]?._eq || query.filter?.[primaryKeyField]?._in || null;

    To be fair, this feels like a hack to me.

  4. Return to state before Add support for read hooks on items #6341, as there's no proper fix at the moment.

Would love to hear your opinions.

Fixes #6622

@MoltenCoffee
Copy link
Contributor Author

MoltenCoffee commented Jul 5, 2021

As discussed in #6622 this hook is now placed in readByQuery, and the items key is replaced with query. The documentation has been updated with this information as well.

@MoltenCoffee MoltenCoffee marked this pull request as ready for review July 5, 2021 13:20
@benhaynes
Copy link
Sponsor Member

Thank you, @MoltenCoffee !!! ❤️

@rijkvanzanten
Copy link
Member

Liked it so much, approved it twice ✅✅

@rijkvanzanten rijkvanzanten added this to the v9.0.0-rc.84 milestone Jul 7, 2021
@rijkvanzanten rijkvanzanten changed the title Fix item.read hook Fix item.read hook not firing for readByQuery Jul 7, 2021
@rijkvanzanten rijkvanzanten merged commit 18ef097 into directus:main Jul 7, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read hook doesn't fire on readByQuery
3 participants