mulesoft-labs / raml-for-jax-rs

This project is all about two way transformation of JAX-RS-annotated Java code to RAML API description and back.
Other
296 stars 181 forks source link

Generated java of subtypes won't compile #402

Open vromero opened 5 years ago

vromero commented 5 years ago

When using a RAML that has for instance a response envelope:

myEnvelope that has some members in it, when creating a subtype, eg: EnvelopeForParticularResource it will generate enums and other types again, creating a change in the Java signature and therefore not compiling for the subtype.

See the following example project to reproduce raml-defined-example.tar.gz.

As a dirty workaround I've been able to change the type: mylib.myType for an type: !include ... which does the trick but is a far cry from ideal.

jpbelang commented 5 years ago

I'll look into it on the weekend. Thanks for the example.

jpbelang commented 5 years ago

Ah, yes. Inline types. The gift that keeps on giving :-). And a smattering of duck typing versus strong typing.

This is an exposition of the problems raml-to-pojo has with your spec, but in no means talks about "right" or "wrong" . It's about compromises that were done in raml-to-pojo, and limitation to Java relative to RAML, which is much better suited to JavaScript and its ilk.

Your current spec contains what is, for all intents and purposes this file (api-standards):

types:
  response-envelope:
    type:
      properties:
        status:
          type: string
          enum: [Success, Failure]
          required: true
        error:
          description: Error information for failed requests.
          type: object
          required: false
          properties:
            message:
              type: string
              required: true
              example: Success
            reason:
              type: string
              required: true
            details:
              type: string
              required: false
    properties:
      metadata:
        type: object
        required: false
        properties:
          offset:
            type: integer
          limit:
            type: integer
          resultCount:
            type: integer
          totalCount:
            type: integer
      data:
        description:
        type: any
        required: false

which means that the properties in the inline parent type of response-envelope. When I go through the properties of a type, I try to figure out if the property is an inline type. In this case, both the status and error properties register as new inline types in both ResponseEnveloppe and SalutationsEnveloppe, so I generate two types. These types have no relationship, so java can't apply its covariant return type rules (as the return type from status in ResponseEnveloppe has no relationship with status in SalutationEnveloppe)

All of this comes from the problem that there is no way, in the current parser, to figure out what is an inline type, which makes the whole thing rather error prone.

I also can't do inheritance from inline types, I think. This would sort of double all the problems that inline types cause.

This almost works. We'll get to the last issue in a second.

#%RAML 1.0 Library

types:
  Status:
    type: string
    enum: [Success, Failure]
    required: true
  Error:
    description: Error information for failed requests.
    type: object
    required: false
    properties:
      message:
        type: string
        required: true
      reason:
        type: string
        required: true
      details:
        type: string
        required: false
  MetaData:
    type: object
    required: false
    properties:
      offset:
        type: integer
      limit:
        type: integer
      resultCount:
        type: integer
      totalCount:
        type: integer

  StatusHolder:
    properties:
      status: Status
      error: Error

  response-envelope:
    type: StatusHolder
    properties:
      metadata: MetaData
      data:
        type: any
        required: false

The last issue has to do with the any type being overridden here:

  data:
    description: 
    type: any
    required: false

any maps to java Object. All of this works until you try to override it. Java has covariant return types, but no covariant parameters. This means that when you override a type, getters will be properly overridden, but setters will not be handled properly. The only way to fix this is to have all inherited properties that descent from an any type to never actually expose anything but Object in the setter type so that the getter would be properly typed, but not the setter. The field would have the proper type but then be subject to some form of runtime error. (BTW, this issue applies to overriding types in general vis-a-vis raml to java).

This I'd rather do through some form of plugin.

jpbelang commented 5 years ago

Oh, I've just noticed I've moved the "required" fields to the types, which is useless in types but important in properties. Just so you know.

vromero commented 5 years ago

Hi @jpbelang,

Thanks for your time, very appretiated

Let me try to summarize the type problem in a gist:

Is this something that could be potentially fixed moving to an AMF based parser as it would hopefully remove the root cause?

In regards of the any. Is there any reason to necessarily try to leverage a single java method for the overrides? Given that we are talking about jax-rs not just plain java (probably even for java only) and tricks could be done with the annotations, can't just a method name suffix be added?

jpbelang commented 5 years ago

Yes that a pretty good summary + the inlining problems.

I have some hope that AMF will do a better job than the current parser a keeping the "unique identity" of a given type. I'm looking into this. However, changing non java class related attributes (such as annotations) might still result in a new type. I'll see.

The simplest way in my opinion to make the covariance problems disappear would be to remove the setters and to delegate the creation to a builder. Along those lines "overriding" raml properties into a similar yet different method ("setXXXForParentClass", "setXXXForThisClass" and "setXXXForAnotherClass") might be an alternative but maybe tricky in multiple inheritance situations. And you'd have to use implementations.

You might be able to do it with the current implementation with a plugin. I know some people have plugged in lombok annotations. With those, and removing setters, it might work.