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

Extending SchemaBase is not producing object with values #27

Closed
ispyinternet opened this issue Aug 8, 2022 · 6 comments
Closed

Extending SchemaBase is not producing object with values #27

ispyinternet opened this issue Aug 8, 2022 · 6 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@ispyinternet
Copy link

ispyinternet commented Aug 8, 2022

fastest-validator: 1.12.0

When I follow the example to extend SchemaBase to use the constructor get a new object, the property values are always undefined, (except any properties that are on the original object but not in the schema - they remain untouched).

i.e.

@Schema()
class Entity extends SchemaBase { // BTW I think the docs have a typo, const should be class
  @Email()
  email: string;
}

const entity = new Entity({ email: '[email protected]' });
console.log(entity); // Entity { email: undefined }

I put some console.logs into the src at https://github.com/AmauryD/fastest-validator-decorators/blob/master/src/index.ts#L148 and can confirm at this point the object has all the values. But somewhere they are getting lost.

ispyinternet added a commit to ispyinternet/fastest-validator-decorators that referenced this issue Aug 10, 2022
ispyinternet added a commit to ispyinternet/fastest-validator-decorators that referenced this issue Aug 10, 2022
@AmauryD
Copy link
Owner

AmauryD commented Aug 10, 2022

Hello,

It's strange indeed, i use the SchemaBase syntax in my projects but i don't have this kind of behavior on my side.
Thank's for submitting a fix, but i'll check on my side if there's not another way to fix the problem. By taking a quick glance, your pull requests seems a bit "hacky" to me atm.

I'll take a closer look at it at the end of the week. I'll keep you informed !

@AmauryD AmauryD added bug Something isn't working good first issue Good for newcomers labels Aug 10, 2022
@AmauryD AmauryD assigned AmauryD and unassigned AmauryD Aug 10, 2022
@ispyinternet
Copy link
Author

Great thanks

@AmauryD
Copy link
Owner

AmauryD commented Aug 14, 2022

The problem

class SchemaBase {
    constructor(props: any) {
       Object.assign(this, props);
    }
 }
 
 class Test extends SchemaBase {
   prop!: string; // undefined
 }
 const email = new Test({prop: "[email protected]"});
 
 console.log(email); // <-- Test { prop: undefined }

The reason it is not working is that the parent constructor is called before the child constructor. The defined properties in the child override the parent properties with undefined.

The behavior is well described in this StackOverflow post : https://stackoverflow.com/questions/56790983/object-assigned-in-base-class-does-not-change-the-child-class

Why it was working in my projects :

class Test extends SchemaBase {
  declare prop: string; // does not exists
}

I use declare instead of the !. The difference is that ! seems to create the property with undefined and declare just tells typescript that the property will exists, so it is not overridden by the child class.

Why it is working in the tests

It seems that the ! in the tests behaves like the declare keyword for an unknown reason. It may depends on the typescript transpiler.

The solution

There are 3 ways to solve the problem :

  • use declare

  • don't use the SchemaBase syntax

  • The code below

import { Email, Schema, SchemaBase } from "fastest-validator-decorators";

function PatchConstructor () {
  return function _DecoratorName<T extends {new (...args: any[]): {}}>(constr: T) {
    if (constr.prototype instanceof SchemaBase) {
      return class extends constr {
        constructor (...args: any[]) {
          super(...args)
          Object.assign(this, args[0]);
        }
      }
    }
    return constr;
  }
}

@PatchConstructor()
@Schema()
class Test extends SchemaBase {
  @Email() 
  prop: string;
}

const email = new Test({prop: "[email protected]"})

console.log("email class before validation", email);

const result = email.validate();

console.log("validation result", result);

console.log("email class after validation", email);

I'll probably apply a similar patch to the Schema constructor.

export function Schema (schemaOptions?: StrictMode | SchemaOptions, messages = {}): any {
  return function _Schema<T extends {new (): any}>(target: any): any {
    /**
     * Support old way of assign schema options
     */
    schemaOptions = typeof schemaOptions === "boolean" ||  typeof schemaOptions === "string" ? {
      strict : schemaOptions
    }: schemaOptions;

    if (target.prototype instanceof SchemaBase) {
      target = class extends target {
        constructor (...args: any[]) {
          super(...args);
          Object.assign(this, args[0]);
        }
      };
    }

    updateSchema(target.prototype, "$$strict", schemaOptions?.strict ?? false);
    if (schemaOptions?.async !== undefined) {
      updateSchema(target.prototype, "$$async", schemaOptions.async);
    }
  
    const s = getSchema(target);
    const v = new FastestValidator({ useNewCustomCheckerFunction: true, messages });

    /**
     * Make a copy of the schema, in order to keep the original from being overriden or deleted by fastest-validator
     * $$async key is removed from schema object at compile()
     * https://github.com/icebob/fastest-validator/blob/a746f9311d3ebeda986e4896d39619bfc925ce65/lib/validator.js#L176
     */
    Reflect.defineMetadata(COMPILE_KEY, v.compile({...s}), target);
    

    return target;
  };
}

With this patch the tests pass except one : "should preserve the constructor name on instancies". i'm still thinking how to solve that problem.

I will maybe remove the SchemaBase syntax in version 2.x. And let the user deal with the way he assigns the properties on the class.

@AmauryD
Copy link
Owner

AmauryD commented Aug 14, 2022

I created a new release named 1.3.1-beta.0. You can check if it fixes the problem on your side.

Your Pull Request #28 was working fine in the tests but the properties were lost until the Object was validated. So it'll probably break some things in production.

I added a test for this case :

it("SchemaBase with @Schema should override already defined properties", () => {

@ispyinternet
Copy link
Author

Hi yes this works. If you were to remove SchemaBase in the future is it just a case of...

const data = { ...props };
const mySchema = new DataSchema();
Object.assign(mySchema, data);

I mean that is ok, but as it stands when extending SchemaBase its super clean and succint.

@AmauryD
Copy link
Owner

AmauryD commented Sep 3, 2022

I'm still not sure about removing the SchemaBase or not ... We will see in the future !
I'm closing the ticket, thank you for your contribution !

@AmauryD AmauryD closed this as completed Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants