Allow JsonSubTypes to be interfaces/abstract classes

[glentakahashi]

By moving the check for interfaces and abstract classes into the getTypeName method as a fallback, the generator will now properly work for JsonSubTypes that are immutables. Previously, if your JsonSubTypes were immutables, those types would get improperly dropped from the taggedUnionClasses.

Also fixes a test case where Quadrilaterals should be type-restricted to only "square" and "rectangle"

[vojtechhabarta]

Thanks for your PR.

I was thinking that in the end some non-abstract classes are needed but if you have use case for abstracts or interfaces lets do it. It would be good if you could add some test with your scenario because otherwise somebody (including me) could refactor this out sometimes in the future.

I also thought that allowing interfaces to be part of tagged unions could cause problems because of multiple inheritance but I tried your change with diamond class hierarchy and it seems it behaves reasonably. I will commit test for it after the merge.

BTW I like that the diff of your PR is really small and there is nothing extra.

[glentakahashi]

Added a test for immutables, where Square is a normal class, Rectangle is an abstract class, and Circle is an interface. Might be redundant to the current TaggedUnionsTest, so let me know if you think it's worth including.

[vojtechhabarta]

Thanks for the test. It doesn't matter that it has similar structure as other tests but it is valuable that it shows how interfaces are used in tagged unions in combination with Immutables library.

[glentakahashi]

@vojtechhabarta can you tag a release for the latest head? Would love to start using this in our proct

[vojtechhabarta]

Released v1.26.333.

[vojtechhabarta]

@sayresVT I was thinking this change will not break much code. Do you have some example what causes the problem? And do you think it affects more people?

I would create configuration switch and set default to original behavior. I want typescript-generator to be dependable so it should provide reasonably good backward compatibility.

On the other hand I would like to provide this functionality to people without any configuration effort.
@glentakahashi what do you think?

[glentakahashi]

Yea didn't realize this would break anything, I thought it would be an entirely additive change. Let me know if it breaks and I can try and make a PR to fix it.

[sayresVT]

I'll try to add a sscce test based upon the issue we're seeing

[sayresVT]

I added the following unit test that I believe demonstrates the same issue we're seeing with some of the generated ts output: Q-Free-OpenRoads/typescript-generator@bd16ac5

It appears that some interfaces are being generated without the discriminant property but then extending two parent interfaces that have incompatible discriminant properties such as:

export interface Child extends ParentA, ParentB {
  // ts error, 'kind' property does not match for ParentA & ParentB
}

export interface ParentA extends RootParent {
  kind: 'Type1' | 'Type2' | 'Type3';
}

export interface ParentB extends RootParent {
  kind: 'Type1' | 'Type2';
}

export interface RootParent {
  kind: 'Type1' | 'Type2' | 'Type3' | 'Type4';
}

export interface ParentC<GenericType> extends RootParent {
  // Also saw one case where the same generated 'kind' property
  // was generated with the type of string instead of a string literal.
  // Currently unable to reproduce this case in a test
  kind: string;  
}

export interface Type1 extends Child {
  kind: 'Type1';
}
// etc.

[glentakahashi]

@sayresVT, I believe the expected output from your test is incorrect. INamedQuadrilateral3 doesn't need the kind: "rectangle"; specification because it already inherits that correctly from IQuadrilateral3 and it would be redundant

[glentakahashi]

As for the Child error case, I couldn't understand what the underlying java was that was incorrectly parsing as that. Do you mind sharing the Java structure that output the incorrect ParentC as well as the incorrect Child. I couldn't get either of them to fail in scenarios that I tried.

[vojtechhabarta]

Yes, inherited properties with same name and type are removed from derived class (ModelCompiler.removeInheritedProperties()).

This removing was originally implemented because Jackson returns all properties (own and inherited) of beans and I didn't want to repeat one property in all descendants.

We could emit discriminant property always but this would require extra code and it would be an exception from the rule.

[sayresVT]

@glentakahashi adding the kind: "rectangle"; specification to INamedQuadrilateral3 was just a guess at what the correct typescript should possibly be. Without it, the generated typescript is invalid because the two inherited properties from IShape3 (kind: "circle" | "rectangle";) and IQuadrilateral3 (kind: "rectangle";) are incompatible. I'll see if I can figure out the classes that are causing the issue observed in ParentC.

@vojtechhabarta I agree that it seems preferable to avoid repeating the property in all descendants. In our case, it really only occurred in 3-4 interfaces that happened to inherit two parent interfaces with incompatible discriminant properties

[glentakahashi]

Ah apologies, I didn't try to compile it, I see what you mean. I think this should be fixable, I'll try and get something out today.

[glentakahashi]

@sayresVT should be fixed in #153

[vojtechhabarta]

I was experimenting with Q-Free-OpenRoads@bd16ac5 test and #153 PR and I found out that removing inherited discriminant properties is really non-intuitive. So I changed my mind and implemented special handling for these properties so that they are preserved in inherited interfaces.

This change also fixes generated TypeScript code in mentioned test so #153 should not be needed. @glentakahashi do you think we can close it?

[glentakahashi]

I like this solution the best! 👍

Closed out my other PR