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

Map from enum #421

Closed
RobbinBaauw opened this issue Oct 3, 2019 · 12 comments
Closed

Map from enum #421

RobbinBaauw opened this issue Oct 3, 2019 · 12 comments

Comments

@RobbinBaauw
Copy link
Contributor

RobbinBaauw commented Oct 3, 2019

Currently, as far as I could find in the codebase, any map is turned into { [index: string]: ...}, as visible here:

if (Map.class.isAssignableFrom(javaClass)) {
final Result result = context.processType(parameterizedType.getActualTypeArguments()[1]);
return new Result(new TsType.IndexedArrayType(TsType.String, result.getTsType()), result.getDiscoveredClasses());
}

But it would be nice if a Map<SomeEnum, Any> could get mapped into { [index in keyof typeof SomeEnum]: ...}. This would allow you to do typesafe accesses.

Edit: This seems to be fully possible (using enums mapped to a custom string value, mapped to union type and mapped to key name).

Example 1:

enum Test {
    TEST1,
    TEST2
}

Map<Test, String> x

maps to

enum Test {
    TEST1 = "TEST1",
    TEST2 = "TEST2"
}

{ [index in Test]: string}

// Could be initialized as
... = {
   TEST1: "somethingTEST1",
   TEST2: "somethingTEST2",
}

Example 2:

enum Test {
    TEST1("a"),
    TEST2("b")

    @JsonValue
    private String value;

    ...constructor
}

Map<Test, String> x

maps to

enum Test {
    TEST1 = "a",
    TEST2 = "b"
}

{ [index in Test]: string}

// Could be initialized as
... = {
   a: "somethingA",
   b: "somethingB",
}

Example 3:

enum Test {
    TEST1("a"),
    TEST2("b")

    @JsonValue
    private String value;

    ...constructor
}

Map<Test, String> x

maps to

type Test ="a" | "b";

{ [index in Test]: string}

// Could be initialized as
... = {
   a: "somethingA",
   b: "somethingB",
}

In the future this could become even better: microsoft/TypeScript#26797.

Is this a feature that could be in the library @vojtechhabarta? I'm willing to implement it

@vojtechhabarta
Copy link
Owner

Thanks for this suggestion. It looks good but I think { [index in Test]: string } type means object with all properties from Test. So this example

type Test = "a" | "b";

const x: { [index in Test]: string } = {
    a: "somethingA",
    b: "somethingB",
}

works well but

const y: { [index in Test]: string } = {
    a: "somethingA",
}

doesn't work (missing property b).

So if I understand it correctly we would need { [index: Test]: string } syntax (there is : instead of in) which is not currently supported in TypeScript but may be added in the future (microsoft/TypeScript#26797).

We would also need to ensure that this feature is not applied when EnumMapping.asNumberBasedEnum is used because keys in JSON can only be strings (not numbers).

@RobbinBaauw
Copy link
Contributor Author

It looks good but I think { [index in Test]: string } type means object with all properties from Test.

Yes it does. This can be fixed by using { [index in Test]?: String }. See the PR for mapped types: microsoft/TypeScript#12114. I think that is indeed the behavior we are looking for.

vojtechhabarta added a commit that referenced this issue Oct 15, 2019
- generating Mapped types like `{ [P in SomeEnum]?: SomeValue }`
- Maps with enums involving numbers still tranformed to indexed types
@vojtechhabarta
Copy link
Owner

Yes, you are right. It looks good so I implemented it. I did it myself because I wanted to go through different edge cases (mainly cases involving numbers in enums). When implementing this feature I also encountered and fixed some bugs.

Could you please try it?

@RobbinBaauw
Copy link
Contributor Author

RobbinBaauw commented Oct 15, 2019

Thanks! I found a small thing.

First off, when you try to convert the following:

interface TestInterface {
    String test();
}

enum TestEnum implements TestInterface {
    A,
    B;

    @Override
    public String test() {
        return name();
    }
}

class Test<X extends Enum<X> & TestInterface> {
    public List<Map<X, ?>> testMap;
}

class TestWithEnum extends Test<TestEnum> {
}

you get

export interface Test<X> {
    testMap: { [index: string]: any }[];
}

export interface TestInterface {
}

export interface TestWithEnum extends Test<TestEnum> {
    testMap: { [P in TestEnum]?: any }[];
}

export const enum TestEnum {
    A = "A",
    B = "B",
}

Here you see that interface Test<X> doesn't use the generic X, even though the type constraint tells us it will be an enum. The hard thing here is that taking care of this would mean having to tell TypeScript that the generic will be an eum (to allow for P in Enum syntax), which kind of exists but not really: microsoft/TypeScript#30611

Some other issues I found (which are not directly to this issue I think):

  • JAX RS with generic #219 doesn't seem to work for Spring MVC. If I convert the same codebase to JAX-RS generics are resolved
  • There still seem to be a few issues with respect to resolving generics:
abstract class GenericService<X, Y> {

    @POST
    open fun list(test: Map<X, Y>): String {
        throw Exception()
    }
}

@Path("")
class TestWithEnum : GenericService<String, String>()

yields

    list(arg0: { [index: string]: Y }, options?: O): RestResponse<string> {
        return this.httpClient.request({ method: "POST", url: uriEncoding``, data: arg0, options: options });
    }

Notice the unresolved Y

  • It seems that overriding a service method (in both Spring and JAX-RS) doesn't work. If you take the previous example and change the following:
@Path("")
class TestWithEnum : GenericService<String, String>() {

    override fun list(test: Map<String, String>): String {
        println("Do something")
        return super.list(test)
    }
}

there won't be any notion of the function whatoever, nothing is generated at all. I'm sure that in JAX-RS this is possible (see this):

JAX-RS annotations MAY be used on the methods and method parameters of a super-class or an implemented interface. Such annotations are inherited by a corresponding sub-class or implementation class method provided that method and its parameters do not have any JAX-RS annotations of its own. Annotations on a super-class take precedence over those on an implemented interface. If a subclass or implementation method has any JAX-RS annotations then all of the annotations on the super class or interface method are ignored. E.g.:

In Spring MVC it generates duplicate methods:

    @RestController
    public static class GreetingController extends TestController {

        @Override
        public Greeting greeting(String name) {
            return super.greeting(name);
        }
    }

    public static abstract class TestController {

        private static final String template = "Hello, %s!";
        private final AtomicLong counter = new AtomicLong();

        @RequestMapping("/greeting")
        public Greeting greeting(@RequestParam(value="name", defaultValue="World") String name) {
            return new Greeting(counter.incrementAndGet(), String.format(template, name));
        }
    }

yields

    /**
     * HTTP GET /greeting
     * Java method: cz.habarta.typescript.generator.sample.spring.SpringTestApplication$GreetingController.greeting
     */
    greeting$GET$greeting(queryParams?: { name?: string; }, options?: O): RestResponse<Greeting> {
        return this.httpClient.request({ method: "GET", url: uriEncoding`greeting`, queryParams: queryParams, options: options });
    }

    /**
     * HTTP GET /greeting
     * Java method: cz.habarta.typescript.generator.sample.spring.SpringTestApplication$GreetingController.greeting
     */
    greeting$GET$greeting(options?: O): RestResponse<Greeting> {
        return this.httpClient.request({ method: "GET", url: uriEncoding`greeting`, options: options });
    }

@vojtechhabarta
Copy link
Owner

That's a lot of feedback 😏
My comments:

  • Generics are resolved differently for JAX-RS and Spring - JAX-RS is resolved by typescript-generator itself while Spring is resolved using Spring libraries.
  • I will check the problem with "unresolved Y".
  • Annotation inheritance in JAX-RS is not currently implemented.
  • Currently all methods (private, inherited, etc.) are considered in Spring REST controllers. Right now I am not sure how exactly Spring behaves (maybe we can consider just public methods)?
  • We can discuss any of these points in separate issues if you think it's worth it.

@RobbinBaauw
Copy link
Contributor Author

Thanks!

In that case the generics resolving capabilities of typescript-generator are far superior to Spring's own :)

I think to this issue only the first thing I mentioned is directly related (though probably very hard to solve nicely), and maybe the unresolved Y.

Annotation inheritance in JAX-RS might be worth another issue, but the Spring issues seem to be a lot of work in order to get to the same level as JAX-RS (and thus maybe are not worth it)?

@vojtechhabarta
Copy link
Owner

In that case the generics resolving capabilities of typescript-generator are far superior to Spring's own :)

I wouldn't say it this way 😏. It is rather the way how typescript-generator uses Spring libraries.

Regarding prioritization of issues I would say that Kotlin nullability could be very useful for people using Kotlin. And also compatibility with Java 13 will be important soon.

@vojtechhabarta
Copy link
Owner

I tried to rewrite your example with "unresolved Y" to Java but it worked correctly. So it is probably something related to Kotlin. I can return to this later when implementing Kotlin nullability.

@olegshtch
Copy link
Contributor

Here you see that interface Test<X> doesn't use the generic X, even though the type constraint tells us it will be an enum. The hard thing here is that taking care of this would mean having to tell TypeScript that the generic will be an eum (to allow for P in Enum syntax), which kind of exists but not really:

Maybe something like #389 will be suitable here.

@rajesh99
Copy link

rajesh99 commented Oct 19, 2019

I am seeing this commit. I dont' follow completely. However, I am looking for example 2 as illustrated by @RobbinBaauw . Will this work with the commit.

Yes, you are right. It looks good so I implemented it. I did it myself because I wanted to go through different edge cases (mainly cases involving numbers in enums). When implementing this feature I also encountered and fixed some bugs.

Could you please try it?

@RobbinBaauw
Copy link
Contributor Author

RobbinBaauw commented Oct 19, 2019

Yes that does work, it's my fault this issue became bigger than it was supposed to be 😅

@vojtechhabarta
Copy link
Owner

Released in version 2.18.565.

RobbinBaauw added a commit to RobbinBaauw/typescript-generator that referenced this issue Nov 10, 2019
Fixed the following:
- Generics resolving in both the return type and method arguments
- PathVariable being string when used without value
- Inheritance giving double methods (could be useful for JAX-Rs too)
vojtechhabarta pushed a commit that referenced this issue Nov 13, 2019
* Fixed a bunch of Spring errors (#437 & #421)

Fixed the following:
- Generics resolving in both the return type and method arguments
- PathVariable being string when used without value
- Inheritance giving double methods (could be useful for JAX-Rs too)

* Fixed checkstyle errors & added extra test case

* Added support for bridged methods (used by Kotlin)

* Fixed dangling semicolon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants