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

preventChanges hook should support dot notation #728

Open
claustres opened this issue Aug 22, 2023 · 2 comments
Open

preventChanges hook should support dot notation #728

claustres opened this issue Aug 22, 2023 · 2 comments

Comments

@claustres
Copy link

Steps to reproduce

Setup the hook like this on a service:

before: {
    patch: [preventChanges(true, ['quotas'])]
  }

Expected behavior

If quotas is actually a nested object and I perform a patch like this using the mongo adapter service.patch(id, { 'quotas.members': 200 }), I expect the hook to raise an error.

Actual behavior

The hook does not filter data properties targetting a prevented field when using dot notation, neither raises an error.

It is not necessarily a bug as one could probably avoid this by registering all available nested properties like this:

before: {
    patch: [preventChanges(true, ['quotas.members'])]
  }

However, in some cases, it is not possible to know all properties upfront. Moreover, it seems to me a convenient way (or shortcut) to discard all properties of a nested object.

We implemented it on our side for now if it can help and seems relevent for this module as well. If so we could try to make a PR but welcome any feedback first.

@fratzinger
Copy link
Collaborator

Nice addition. I don't use dot-notation and I don't use mongo myself but I see the use case. But you maybe want to prevent changes for preventChanges(true, ['quotas']) but you want to pass changes for preventChanges(true, ['quotas.members']).

Also maybe you have a column with dot-notation. In postgres e.g. it's possible to define a column as 'quotas.members'. So you could have a column 'quotas' as integer and 'quotas.members' as integer in the same table. It's does not sound like a good design but it's possible.

So for maximum flexibility you maybe should define it like this:

before: {
    patch: [preventChanges(true, [{ name: 'quotas', nested: true }])]
}

where nested: true is your described behavior and nested: false would pass for service.patch(id, { 'quotas.members': 200 })

What do you think?

@claustres
Copy link
Author

claustres commented Aug 22, 2023

For sure we could manage it this way but I am not able to get your point and if it's necessary:

  1. quotas is an atomic property and preventChanges(true, ['quotas']) will do the work.
  2. quotas is a nested object and you'd like to prevent access to the whole subobject so that preventChanges(true, ['quotas']) should do the work as well.
  3. quotas is a nested object and you'd like to prevent access to some of the subobject properties so that preventChanges(true, ['quotas.xxx', 'quotas.yyy']) should do the work.

As 1 and 2 are mutually exclusive I don't see the need to distinguish with a nestedflag but I might be wrong, let me know .

Another option is to be able to provide field names as a regex like this preventChanges(true, [/^quotas/]). and rely on test() to find matching data payload keys in this case (we could check if the field name is a regex object or a simple string I guess).

Yet, I wonder if we also need to support the following construct in Feathers for some adapters, which can make things harder as we need to 'dotify' the payload first:
service.patch(id, { quotas: { members: 200 } })

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

No branches or pull requests

2 participants