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

Add ability to validate/modify property values as they are set #3567

Closed
lagnat opened this issue Oct 6, 2022 · 16 comments
Closed

Add ability to validate/modify property values as they are set #3567

lagnat opened this issue Oct 6, 2022 · 16 comments
Labels
enhancement New feature or request

Comments

@lagnat
Copy link

lagnat commented Oct 6, 2022

Is your feature request related to a problem? Please describe.
When assigning values to an entity instance, we'd like to be able to provide a validation function that can alter the supplied value if it wants. Most of the time it'll be something like making sure some fields are uppercase and other times we might want to reject a value if it doesn't pass some validation.

Describe the solution you'd like
My current thinking is to have a validation function in the @Property decorator that accepts and returns the value.

Describe alternatives you've considered
I spent some time trying to code my way out of this with get and set functions but they are overwritten during hydration.

Additional context
None

@lagnat lagnat added the enhancement New feature or request label Oct 6, 2022
@B4nan
Copy link
Member

B4nan commented Oct 6, 2022

You can use custom mapped types for this, or no?

https://mikro-orm.io/docs/custom-types

Or when exactly do you want to validate things?

Most of the time it'll be something like making sure some fields are uppercase

If you mean validating the keys, that is a bit out of scope, just define your own funciton that validates the keys. For validating the values, mapped types should do the job.

@lagnat
Copy link
Author

lagnat commented Oct 6, 2022

I considered custom types. I use one already (they're fantastic btw) but if I have an entity with 20 string properties that I want to validate, that would call for 20 custom types. Here's an example, just to make sure we're on the same page. If you feel it's out of scope, I would be disappointed but life goes on.

    @Property({
            fieldName: 'foo',
            type: 'string',
            validator: (val) => {
                    val = (val || '').toUppercase();
                    if (!['ABC', 'DEF'].includes(val)) throw 'gtfo';
                    return val;
            }
    })
    public foo: string;

    ....

    entity.foo = 'abc';
    console.log(entity.foo);   // FOO

    entity.foo = 'bar';  // throws

I tried to code this directly on the entity class but the hydrator overwrites the properties.

@B4nan
Copy link
Member

B4nan commented Oct 6, 2022

If you want to validate them in 20 different ways, yes. Otherwise one type will do the job just fine. You can even have multiple instances of a single type class, with different parameters.

Btw for this purpose you can use enums too, which will give you schema level validation, much better if you ask me.

I tried to code this directly on the entity class but the hydrator overwrites the properties.

Provide some code, I dont know what you tried. You can have a property defined as get/set pair.

@B4nan
Copy link
Member

B4nan commented Oct 6, 2022

I think I know what you mean with the setters. If you have a value in database, its hydrated as a value - and that is expected behaviour. Calling the setting with internal value seems to be wrong idea (imagine a setter that adds something to the string, it would always produce update queries), and we can't set the value and have it defined as a setter at the same time - thats simply not possible in JS. What you can do is to have a regular property to store value, and a virtual get/set pair to work with it. But its all very quirky, you should go with custom types (or maybe with enums).

I will close this, the example you provided is the exact use case for them. Only difference is that the validation/mutation takes place on flush and not when setting the value, which should be fine.

@B4nan B4nan closed this as not planned Won't fix, can't repro, duplicate, stale Oct 6, 2022
@lagnat
Copy link
Author

lagnat commented Oct 7, 2022

The back-story here is that we're switching from Sequelize and sequelize models have this capability. I see your point about calling the setter during hydration. Fairly sure Sequelize doesn't do that either. I'm fairly confident there's a valid use case here but I need to better define how it would work. Thanks for the feedback. I'll continue to think about this.

@lagnat
Copy link
Author

lagnat commented Oct 27, 2022

Decided to go with custom types for validation and it's working well with one exception. One side effect, which is probably a good thing, is that I'm now getting validation on custom types used in criteria. The problem I'm having is with like criteria. It seems clear that like criteria and custom types are going to be incompatible when the value is transformed in any non-trivial way. For a string type where I'm only trying to utilize a custom type for validation, the like criteria fails that validation. Consider a validation for IP address and a criteria such as { $like: '192.168.%' }. To make this work, I'd need to know the context in convertToDatabaseValue(). I may choose to disregard validation in the context of criteria.

Like to get your thoughts on this, and thanks!

@lagnat
Copy link
Author

lagnat commented Nov 1, 2022

Just discovered that convertToDatabaseValue() has a third argument that is not documented. fromQuery?: boolean is what I need. If there's any request here, it's to also provide the operator so that I can make an informed decision about running validation code.
Edit: I'm wrong about what fromQuery means. Seems to mean that the caller is the QB, so that doesn't help me.

@lagnat
Copy link
Author

lagnat commented Nov 1, 2022

This is what I'm after. A way to know when to skip validation in a custom type, or when to throw an exception when someone is trying to use a like comparator on an incompatible custom type. For example, consider a custom type called EpochType that converts and ISO date string into an epoch time. Using like would not be possible.

  static processCustomType<T>(prop: EntityProperty<T>, cond: FilterQuery<T>, platform: Platform, key?: string, fromQuery?: boolean): FilterQuery<T> {
    if (Utils.isPlainObject(cond)) {
      return Object.keys(cond).reduce((o, k) => {
        if (Utils.isOperator(k, true) || prop.referencedPKs?.includes(k)) {
          o[k] = QueryHelper.processCustomType(prop, cond[k], platform, k, fromQuery);
        } else {
          o[k] = cond[k];
        }

        return o;
      }, {} as ObjectQuery<T>);
    }

    if (Array.isArray(cond) && !(key && ARRAY_OPERATORS.includes(key))) {
      return (cond as ObjectQuery<T>[]).map(v => QueryHelper.processCustomType(prop, v, platform, key, fromQuery)) as unknown as ObjectQuery<T>;
    }

-    return prop.customType.convertToDatabaseValue(cond, platform, fromQuery);
+    return prop.customType.convertToDatabaseValue(cond, platform, fromQuery, key);
  }

@B4nan
Copy link
Member

B4nan commented Nov 2, 2022

I don't like to add more parameters in there, in fact, since v5.5 we dont even need the platform param, as it's available on the type instance (this.platform). But we can turn the last parameter into a context object, the fromQuery param is not documented and considered internal, so I don't see that as a breaking change.

@B4nan B4nan reopened this Nov 2, 2022
@lagnat
Copy link
Author

lagnat commented Nov 2, 2022

That would be fantastic. Thanks again (and again)!

@B4nan B4nan closed this as completed in a933e98 Nov 2, 2022
@lagnat
Copy link
Author

lagnat commented Nov 4, 2022

One potential open issue. You said that platform is available as this.platform however that does not appear to be the case and I'm guessing it's only if mikro constructs the type instance. Since I'm using custom types for validation, I'm passing validation parameters in the constructor. e.g.

    @Property({
        fieldName: 'name',
        type: new MyStringType({
            not: /^-|-$|^[0-9]+$/,
            is: /^[0-9a-z-]+$/,
            len: [1, 63],
        }),
    })

I believe you refer to this problem in your V6 Ideas post but wanted to make sure I'm not creating an issue for myself today.

@B4nan
Copy link
Member

B4nan commented Nov 4, 2022

I was wrong, there is no platform property at all, just prop and meta. I might rework how the type instances are created in v6, but that's still just in the thinking phase, it may stay the way it is now...

@lagnat
Copy link
Author

lagnat commented Nov 4, 2022

prop and meta are not there either.

@B4nan
Copy link
Member

B4nan commented Nov 4, 2022

Depends on when you check, they are added during discovery. They will be there only for runtime convertion methods.

https://github.com/mikro-orm/mikro-orm/blob/master/packages/core/src/metadata/MetadataDiscovery.ts#L986-L987

@lagnat
Copy link
Author

lagnat commented Nov 4, 2022

Ah.. that was added in 5.5.0. Fantastic! I'm blocked from upgrading to 5.5.0 because of a regression that you've already fixed for the next release.

@B4nan
Copy link
Member

B4nan commented Nov 4, 2022

You can use dev version, every commit to master is published to NPM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants