discussion: poor handling of errors thrown by target class constructors (like Firebase Timestamp)

[YGKtech]

I've spent some time figuring this problem out, and since the repo isn't being actively maintained right now I figure the best I can do to give other's access to my findings is to put them in an issue.

In TransformOperationExecutor.ts (.js if you are digging through the compiled code from node_modules like I was), the library tries to figure out how to construct it's target type, and under certain conditions will reach line 94:

newValue = new (targetType as any)();

(or line 86 in the JS version):

newValue = new targetType();

This just blindly calls the target type's constructor with no arguments, which works for classes that don't have real constructors that expect arguments, but doesn't work for classes that have constructors that validate their arguments at runtime.

I think this is probably what the code should be doing in this case, there's no way for it to figure out if the constructor expects arguments, or what it does with them, and it works for many use cases. But there are plenty of cases where it won't work, and those can be handled in a way that will at least give more useful errors.

The Firebase/Firestore SDK's are switching to using their own Timestamp class instead of the native Date class for storing time values, and the constructor to that class expects two numbers, and throws a runtime exception if they are not present and valid.

Ideally, TransformOperationExecutor would wrap the call to the constructor in a try-catch block, so it could print a helpful error message like: "the constructor for the target class threw an error, this class may not be compatible with class-transformer..." then re-throw the caught error. This would make it much more obvious what is going on, and wouldn't change the runtime behavior at all, a strict improvement over the current functionality.

I would gladly submit a PR for this, but PR's aren't getting reviewed right now so I'm not going to invest my time in that.

For anyone else that runs into this compatibility issue, or something similar, I have found a fairly respectable workaround.

export class User {
  @IsString()
  @IsEmail()
  email!: string;

  @Transform((value: any) => {
    const seconds: number = value._seconds;
    const nanoseconds: number = value._nanoseconds;
    return new Timestamp(seconds, nanoseconds);
  })
  @Type(() => Object)
  created_time!: Timestamp;
}

This example is for the specific types I was working with, but the pattern can be used in many other cases.

I use @Type(() => Object) to bypass the call to the Timestamp constructor, class-transformer will just give me a plain object copy of the source data for that key, Then I use @Transform to hook into the transformation lifecycle, and craft a valid call to the constructor out of the Object.

I've seen a few open issues in this repo that could potentially be helped by this pattern, it may be a good idea to add a note about it to the readme, or add a new decorator that accomplishes the same thing, giving people a simpler way to write their own custom transformers.

I appreciate how useful this library is, combined with class-validator it lets me easily add runtime-validation of dynamic data without having to write a ton of custom code, it's just unfortunate that it isn't being maintained well right now.

[cojack]

@YGKtech thanks for your time to investigate, could you please explain me what of the error messages you've got? Because I was missing this part of the lecture, maybe we will be able to improve error messages a bit.

[YGKtech]

@cojack : I was just seeing the error thrown by the Timestamp constructor, so it's not an error your code is throwing, but one it probably should be catching, since the resulting stack trace doesn't include any of my code, it's all internal methods from your library and the target constructor.

There's a block of code in this library where the constructor function a class can get called with no parameters, this is likely to result in an error for any class that validates it's parameters at runtime, so it should be treated like a fail-able operation and wrapped in a try-catch so you can define the control flow for when an error is thrown.

The error message I end up seeing in my console is this:

 validation error:  Error: Argument "seconds" is not a valid integer.
     at Validator.(anonymous function).values [as isInteger] (/my/project/dir/node_modules/firebase-admin/node_modules/@google-cloud/firestore/build/src/validate.js:99:27)
     at new Timestamp (/my/project/dir/node_modules/firebase-admin/node_modules/@google-cloud/firestore/build/src/timestamp.js:115:18)
     at TransformOperationExecutor.transform (/my/project/dir/node_modules/class-transformer/TransformOperationExecutor.js:86:32)
     at _loop_1 (/my/project/dir/node_modules/class-transformer/TransformOperationExecutor.js:154:45)
     at TransformOperationExecutor.transform (/my/project/dir/node_modules/class-transformer/TransformOperationExecutor.js:178:17)
     at _loop_1 (/my/project/dir/node_modules/class-transformer/TransformOperationExecutor.js:154:45)
     at TransformOperationExecutor.transform (/my/project/dir/node_modules/class-transformer/TransformOperationExecutor.js:178:17)
     at _loop_1 (/my/project/dir/node_modules/class-transformer/TransformOperationExecutor.js:154:45)
     at TransformOperationExecutor.transform (/my/project/dir/node_modules/class-transformer/TransformOperationExecutor.js:178:17)
     at ClassTransformer.plainToClass (/my/project/dir/node_modules/class-transformer/ClassTransformer.js:17:25)

For context, this is happening while transforming an plain JS object to an instance of my User class:

class User extends TransformTargetClass {
  email!: string;
  created_time!: Timestamp;
}

The error is happening because you end up calling the constructor of Timestamp to populate the created_time prop. That constructor function throws an error when called with no parameters. Unfortunately, as you can see in the stack trace, there's no clear indication that this is happening while transforming my User class, or what prop is being targeted. This happens in my authentication middleware on a call to do some database operation, so just looking at the stack trace, it's hard to tell what class I need to troubleshoot.

I think the easiest thing you could add in is something like this:

class-transformer: target class constructor threw error during transformation: 
[insert original error here]

this at least makes it obvious that the error is happening as a result of a call made by class-transformer, which may not be clear depending on the stack trace.

Or, you could go the extra mile and print some context for the transformation that failed:

class-transformer: constructor for target class "Timestamp" threw an error while transforming plain object to class "User", the constructor for "Timestamp" was called while populating the "User" property "created_at".
"Timestamp" may not be compatible with class-transformer's plainToClass method at this time, consider excluding fields using it, or writing a custom transformation handler for it.
[insert original error here]

---

On a related note: In case anyone else is having issues with classes like Timestamp, I've refined my workaround for dealing with this error a bit since I last posted.

My previous strategy of using @Type(() => Object) to bypass the constructor call, then writing my own transformer inside a @Transform decorator works for simple attributes like created_at!: Timestamp but doesn't scale well for complex types like: records_map!: { [key: string]: Timestamp }.

Here's the pattern I've been using:

const CUSTOM_TRANSFORMER_KEY = 'my-metadata-key-custom-transformer';

export function CustomTransformer<T>(transformFn: (value: any, obj: any, transformationType: TransformationType) => any, options?: TransformOptions) {
  return function (target: object, propertyKey: string) {
    Exclude({ toClassOnly: true })(target, propertyKey);
    // Exclude to make sure class-transformer doesn't try to handle this key itself.
    const transformMetadataForClass: TransformMetadata[] = Reflect.getMetadata(CUSTOM_TRANSFORMER_KEY, target.constructor) || [];
    const metadata = new TransformMetadata(target.constructor, propertyKey, transformFn, options || {});
    transformMetadataForClass.push(metadata);

    Reflect.defineMetadata(CUSTOM_TRANSFORMER_KEY, transformMetadataForClass, target.constructor);
  };
}

export abstract class TransformTargetClass {
  public applyCustomTransforms(sourceObject: any): void {
    const transformMetadataForClass: TransformMetadata[] | undefined = Reflect.getMetadata(CUSTOM_TRANSFORMER_KEY, this.constructor);
    if (undefined != transformMetadataForClass) {
      transformMetadataForClass.forEach(transform => {
        this[transform.propertyName] = transform.transformFn(sourceObject[transform.propertyName], sourceObject, TransformationType.PLAIN_TO_CLASS);
      });
    }
  }
}

Custom decorator that lets me define a transform function for a prop, and store it in reflect-metadata, and an abstract class that provides a method for applying those transformations

export class User extends TransformTargetClass {
  email!: string;

  @CustomTransformer((value: SerializedTimestampObject) => {
    const seconds: number = value._seconds;
    const nanoseconds: number = value._nanoseconds;
    return new Timestamp(seconds, nanoseconds);
  })
  created_time!: Timestamp;

  @CustomTransformer((value: { [key: string]: Timestamp }, obj: any, transformationType: TransformationType) => {
    const out: { [key: string]: Timestamp } = {};
    for (const k in value) {
      out[k] = value[k];
    }
    return out;
  })
  records_map!: { [key: string]: Timestamp };
}

class with attributes involving problematic types extends the TransformTargetClass class from above, and uses the decorator to provide custom transformers.

protected buildInstance(args: UnknownObject<T>): T {
    const out = plainToClass(this.class, args);
    if (out instanceof TransformTargetClass) {
      out.applyCustomTransforms(args);
    }
    const validationErrors = validateSync(out);
    if (validationErrors && validationErrors.length > 0) {
      throw validationErrors;
    }
    return out;
  }

Code that actually uses class-transformer to transform plain objects to my data model class checks if the output class extends my TransformTargetClass class, then applies custom transformers if it is.

[YGKtech]

@cojack : just got caught up on the status of the repo, if you and the other new maintainers want to prioritize other fixes, I could try setting up a PR for this sometime in the next week or two.

[MTschannett]

@YGKtech: That be great. The description sounds interesting. Feel free to ping me when you are ready to merge.

[cojack]

@YGKtech can not wait to see your PR!

[YGKtech]

@cojack : should I base my changes on the develop branch or master?

[NoNameProvided]

Hi @YGKtech!

You have found the right solution with the @Transform decorator, however, I don't see why you need to use the @Type(() => Object) line? This work just fine for me:

import { plainToClass, Transform } from 'class-transformer'

class SubClass {

  constructor(requiredParam: any) {
    if(requiredParam == undefined) {
      throw new Error('Ooops');
    }
  }
}

class Example {

  @Transform(value => new SubClass(value))
  x: SubClass;
}

console.log(plainToClass(Example, { x: 'I have it' }));

Is there something I dont see?

Or, you could go the extra mile and print some context for the transformation that failed:

class-transformer: constructor for target class "Timestamp" threw an error while transforming plain object to class "User", the constructor for "Timestamp" was called while populating the "User" property "created_at".
"Timestamp" may not be compatible with class-transformer's plainToClass method at this time, consider excluding fields using it, or writing a custom transformation handler for it.
[insert original error here]

That looks good.

[YGKtech]

@NoNameProvided : I'm not sure why, but when I just provide a @Transform(value => new SubClass(value)) for my use case, the constructor of the prop's target class still ends up being invoked, and @Type(() => Object) suppresses that invocation. That doesn't seem to be the case for all types though, so I'm not sure what's going on there.

I think it has to do with my 'plain' source data actually containing instances of the problematic target class, this may cause the library to take a different path during the conversion. Source data looks like this:

{
  email: 'test@example.com',
  created_time: <instanceof SubClass>
}

[NoNameProvided]

I think it has to do with my 'plain' source data actually containing instances of the problematic target class,

Then why are you not using just @Transform(v => v)?

[YGKtech]

@NoNameProvided : IIRC it's because the argument that gets passed to my @Transform function is a instance of Object, not my actual output type, probably a side effect of me using @Type(() => Object) to bypass the implicit call to the constructor.

[sebastian-zarzycki-apzumi]

This is most bizarre. I have this following code:

const data: UserDTO = plainToClass(UserDTO, { 
  email: 'a@b.com', 
  myDate: new firebase.firestore.Timestamp(1548955423,0)
});
console.log(data);

with UserDTO

export class UserDTO {

  email: string;

  @Transform(DateUtil.firebaseFirestoreDateConversion)
  myDate: Date;

}

Nevermind what is happening in the transformation function. The point is that :

• this code executed in web browser environment (angular app, ionic app), converts the Timestamp just fine - conversion function expects timestamp, converts it to Date and all is good. Constructor for Timestamp is somehow not called.
• this same code executed as Firebase function (in their environment), crashes (it never enters the transformation function). Unfortunately, it is very hard to debug that environment, so I don't know what's going on in detail, but it is definitely connected to Timestamp. I assume the constructor is actually called and there it crashes. So in Firebase functions environment, I have to use the workaround provided by @YGKtech .

Granted, firebase functions and web page doesn't share exactly the same firebase/firestore functions (they use different libs), but something is very weird here. It's as if it was reliant on something in environment (shims?) or in actual node.js.

[sebastian-zarzycki-apzumi]

Sigh. Firebase for JS/web uses a different Timestamp implementation. The functions one (firebase-admin), uses completely different implentation from @google-cloud/firestore. And, indeed, in that implementation, there is validation in constructor and an error is thrown.

Firebase is a hot mess.

[YGKtech]

Firebase is a hot mess.

Yes, yes it is. It's an amazing platform and I happily use it in several production applications, but like so many google products, it is surprisingly janky.

[maxgfr]

Thanks Mamodom for your implementation of the 06/22/19 !!!!

To sum up, its solution is to implement this function :

const transformFirestoreTypes = (obj: any): any => {
   Object.keys(obj).forEach(key => {
      if (!obj[key]) return
      if (typeof obj[key] === 'object' && 'toDate' in obj[key]) {
         obj[key] = obj[key].toDate().getTime()
      } else if (obj[key].constructor.name === 'GeoPoint') {
         const { latitude, longitude } = obj[key]
         obj[key] = { latitude, longitude }
      } else if (obj[key].constructor.name === 'DocumentReference') {
         const { id, path } = obj[key]
         obj[key] = { id, path }
      } else if (typeof obj[key] === 'object') {
         transformFirestoreTypes(obj[key])
      }
   })
   return obj
}

Then, you can write something like this :

const obj: ObjEntity = plainToClass(
                  ObjEntity,
                  transformFirestoreTypes({
                     ...doc.data(),
                  }),
               )

[NoNameProvided]

As mentioned in this thread, the @Transform decorator is needed to convert property values that have classes with non-empty constructors. This is the expected behavior as class-transformer cannot know how to initiate all the classes in the world.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.