class-transformer icon indicating copy to clipboard operation
class-transformer copied to clipboard

Implement class-level @Type decorator polymorphism

Open yazshel opened this issue 5 years ago • 32 comments

Here is an option for improving the polymorphism in class-transformer. Closes #51.

This PR extends the existing @Type() decorator to be usable at class level. This allows the user to decorate a base class, with a custom function to resolve the actual target type for the transform before mapping properties.

This allows polymorphic top-level transformations using plainToClass and classToClass variants, by adding a type mapping function to the parent (for example):

ie.

@Type(
   opts => opts.object.discriminator === 'child1' ? Child1
       : opts.object.discriminator === 'child2' ? Child2
       : Parent
)
class Parent {
  discriminator: 'child1' | 'child2' | 'parent';
  id: number;
  data: string;
}

class Child1 extends Parent {
  child1Data: string;
}

class Child2 extends Parent {
  child2Data: string;
}

const plainChild1 = {
  discriminator: 'child',
  id: 1,
  data: 'mainData',
  child1Data: 'extraChildData',
};

// obj will be a Child1 object 
const obj = plainToClass(Parent, plainChild1);

My main reason for implementing this functionality is for use in transforming and validating polymorphic view models from JSON API request bodies.

This PR also allows for polymorphic transformations of array or map items, such as those detailed in PR #125, but implemented with a different (IMHO more simple) methodology which better fits within the existing class-transformer infrastructure:


const getPhotoType = (value: any) => {
    let resolvedType =
          value.panorama === undefined ? LandscapePhoto
        : value.person !== undefined ? PortraitPhoto
        : value.depth !== undefined ? UnderWater
        : undefined;
    if (resolvedType === undefined) {
        throw new Error(`Unable to determine photo type`);
    }
    return resolvedType;
};
@Type(opts => getPhotoType(opts.object))
export abstract class Photo {
    id: number;
    filename: string;
}
export class LandscapePhoto extends Photo {
    panorama: boolean;
}
export class Person {
  name: string;
}
export class PortraitPhoto extends Photo {
    @Type(() => Person)
    person: Person;
}
export class UnderWaterPhoto extends Photo {
    depth: number;
}
export class Album {
     id: number;
    name: string;
    @Type(() => Photo)
    topPhoto: Photo;
    @Type(() => Photo)
    photos: Photo[];
 }

let album = plainToClass(Album, albumJson);

In this case, both album.topPhoto and the items in album.photos will be instances of one of the Photo subclasses.

I'm happy to do more work on more unit tests and documentation but wanted to check that this solution is acceptable before doing further work on it.

yazshel avatar Oct 01 '18 03:10 yazshel

@yazshel Hi, I have check your PR, but it fail at test, screenshot_20181021_130402

Could you fix these, and we will go forward.

cojack avatar Oct 21 '18 11:10 cojack

It appears that commit ff5fcefb163bde31a70d2f7792fe4022ba6c95c0 has introduced significant changes to MetadataStorage, which break this PR ... I'll try to have a look and fix it over the next few days.

yazshel avatar Oct 24 '18 00:10 yazshel

I've just fixed it - the new Map-based MetadataStorage actually makes this a whole lot easier; I've rebased to the latest version and removed the MetadataStorage change in favour of using MetadataStorage.findTypeMetadata().

yazshel avatar Oct 24 '18 00:10 yazshel

@cojack or @NoNameProvided could one of you also review this.

MTschannett avatar Oct 30 '18 18:10 MTschannett

Still conflicting. Can you rebase it once more @yazshel? At first glance, I like the API surface 👍, but I would like to review the final state, so nothing can go wrong during the rebase/merge.

NoNameProvided avatar Nov 04 '18 20:11 NoNameProvided

Hi, any update about this PR? I really need this feature.

pankyka avatar Dec 04 '18 09:12 pankyka

I've rebased it as requested but it's still pending... The rebase was necessary to work around conflicting changes introduced in PR #125 (for which I originally intended this PR as an alternative method for implementing polymorphic functionality).

It should be working now - all the tests passed at the time I force-pushed the rebased changes. Can it be approved again as originally planned please?

yazshel avatar Dec 04 '18 12:12 yazshel

@NoNameProvided Could you take a look otherwise I'd merge it on sunday

MTschannett avatar Dec 07 '18 09:12 MTschannett

I'm really excited for this one! 👍 Been bashing my head over this very problem.

miking-the-viking avatar Dec 16 '18 15:12 miking-the-viking

Would also love to see this get merged for some stuff I've been working on lately. @MTschannett is there anything still blocking this from getting closed?

WonderPanda avatar Dec 16 '18 18:12 WonderPanda

Hi . No there should be nothing that is blocking this . I forgot to merge it . sorry ^^

MTschannett avatar Dec 16 '18 20:12 MTschannett

I'll merge this PR as soon as i got back to my pc. I can't merge it on my mobile.

MTschannett avatar Dec 16 '18 20:12 MTschannett

It looks like i cannot merge this until a second review has been made. I'm sorry guys. Maybe one of you could make one?

I'm sorry for the delay. Happy new year to everyone :)

MTschannett avatar Jan 03 '19 21:01 MTschannett

It looks completely dead. @MTschannett Can you promote someone of us to reviewer? Just for this PR only. I think we really need this feature.

pankyka avatar Jan 16 '19 12:01 pankyka

@pankyka Sure . I'll try to add you today or tomorrow if that's ok for you.

MTschannett avatar Jan 16 '19 13:01 MTschannett

I'm ready!

pankyka avatar Jan 16 '19 13:01 pankyka

I'm still waiting for your response :)

pankyka avatar Jan 22 '19 15:01 pankyka

@pankyka When is this going to be merged? :)

branimir93 avatar Mar 01 '19 09:03 branimir93

@branimir93 I don't know. I have still not been appointed for review :(

pankyka avatar Mar 01 '19 09:03 pankyka

@MTschannett There's a number of people hoping that this can be merged through. Any chance that you can give us a hand?

WonderPanda avatar Mar 01 '19 20:03 WonderPanda

I do need this feature too for my project! Is it possible to get this in several days or weeks?

dmvlasenk avatar Mar 11 '19 17:03 dmvlasenk

Love to see this merged asap, need this too!

halfmatthalfcat avatar Aug 08 '19 22:08 halfmatthalfcat

I hope this will be merged one time

Djaler avatar Apr 21 '20 08:04 Djaler

@MTschannett What can I do to help this get merged? Would be supremely useful for me right now.

isparling avatar Apr 24 '20 00:04 isparling

Guys I'd say this repo is abandoned, sadly. I've been following this for a while and there is just no progress. #314

patricknazar avatar May 05 '20 11:05 patricknazar

Hi team,

Nice to see a fair bit of commit activity on this project over the past few months 👍

I'm just following up this PR - it's been over 2 years since it was ready to be merged but no second approver for the changes was available, and there are now several merge conflicts due to subsequent changes on master. Is this PR likely to get merged If I fix these? I've put in a fair bit of effort into syncing with breaking changes in master several times previously but would be happy to sync again if I know it's definitely going to make it in.

Cheers,

Timshel

yazshel avatar Oct 15 '20 22:10 yazshel

Hi guys,

I think a lot of people really need this feature. It's really sad to see that the work have already been made but the PR was never merged for no reason..

Can you please answer to @yazshel so that he can know if it worth it to fix the current conflicts ?

Thank you in advance guys :)

ladislas14 avatar Nov 06 '20 11:11 ladislas14

Looks like this PR needs a merge from the latest master. If I can resolve the merge conflict, are there any maintainers who would be willing to review and approve? This feature is sorely needed for almost 2 years.

nathanbabcock avatar Sep 17 '21 19:09 nathanbabcock

Guys please, this one is really needed! Currently it requires a lot of time to implement workarounds and maintain them all over the places to do what this feature can do with a few lines of code.

If there is any help that I can provide in moving it forward, I'm ready to contribute

arthabus avatar Oct 30 '21 10:10 arthabus

A question - defining discriminator on the parent assumes importing subclasses in the parent class which introduces a circular dependency which breaks the entire logic. How do you overcome that?

arthabus avatar Oct 31 '21 13:10 arthabus