DTO Generation Failing for Extensions

[pbtura]

The generation of the DTO classes produces results with invalid syntax for DTOs with inheritance, where the 'properties' config is not explicitly set. If the properties config is missing, the copyOf method of the child DTO will be invalid. If the properties config is set to '*' the constructor of the child class will have duplicate parameters.

example configs from dtonator-features:

Working

AccountDto:
  domain: Account
  properties: id, name
  tessellModel: true
  forceMapperMethods: name

RedAccountDto:
  extends: AccountDto
  domain: RedAccount
  properties: foo
  tessellModel: true

Invalid 'copyOf'

AccountDto:
  domain: Account
  tessellModel: true
  forceMapperMethods: name

RedAccountDto:
  extends: AccountDto
  domain: RedAccount
  tessellModel: true

Duplicate Constructor Parameters

AccountDto:
  domain: Account
  properties: '*'
  tessellModel: true
  forceMapperMethods: name

RedAccountDto:
  extends: AccountDto
  domain: RedAccount
  properties: '*'
  tessellModel: true

[pbtura]

Not sure if this is intended behavior or not, but it looks like the default for DTOs with no 'properties' specified in the config is to exclude all. If this is intended, the documentation needs to be updated.

[stephenh]

Hey. Thanks for filing the issue. I'm not sure the best thing to do here...exclude all by default does seem odd. Maybe include all would be better?

If you want to submit PRs with your suggested fixes, either in code or just documentation, I'll happily accept them.

[pbtura]

I currently have a forked version of the code that I'm playing around with. I was able to solve the first issue I mentioned by adding a 'equals' and 'hashcode' methods to DtoProperty and calling FluentList.unique() on the results of getAllProperties in DtoConfig. The second issue seems like less of a problem. As long as the documentation makes it clear that every Dto config needs a 'properties' entry, then using '*' to indicate 'include all' is fine.

In the process of working on those issue though, I noticed something else. When a Dto is marked as 'extends' the child dto ends up redefining all the fields of the parent Dto. For example, in the dtonator-features project there is a RedAccountDto that extends AccountDto. When the generator is run, the classes end up looking like this:

@javax.annotation.Generated("dtonator")
public abstract class AccountDto {

  public Long id;
  public String name;

  protected AccountDto() {
  }
...
}

@javax.annotation.Generated("dtonator")
public class RedAccountDto extends AccountDto {

  public Long id;
  public boolean foo;
  public String name;

  protected RedAccountDto() {
  }
...
}

Notice how both classes define an 'id' and 'name' property? Is that expected behavior or a bug?

[stephenh]

Hey. You are right, AccountDto and RedAccountDto both having id/name seems wrong. I'm ~80% sure dtonator doesn't do this normally (e.g. could this be a behavior introduced by your changes)?

E.g. where it adds fields is here:

https://github.com/stephenh/dtonator/blob/master/dtonator/src/main/java/com/bizo/dtonator/GenerateDto.java#L79

And it uses getProperties() which does not include any inherited properties, e.g. here:

https://github.com/stephenh/dtonator/blob/master/dtonator/src/main/java/com/bizo/dtonator/config/DtoConfig.java#L31

Where as the behavior you're seeing would happen if line 79 did for property in dto.getAllProperties:

https://github.com/stephenh/dtonator/blob/master/dtonator/src/main/java/com/bizo/dtonator/config/DtoConfig.java#L66

---

a 'equals' and 'hashcode' methods to DtoProperty and calling FluentList.unique() on the results of getAllProperties in DtoConfig

That is interesting, and kind of makes sense, but I'm wondering why there were duplicate properties in getAllProperties in the first place? That is not really expected, so I think a potentially easier fix would be to keep that from happening.

I wonder if the issue you're seeing is that properties: * does just not work well with inheritance...e.g. it could be that the oracle.getProperties:

https://github.com/stephenh/dtonator/blob/master/dtonator/src/main/java/com/bizo/dtonator/properties/ReflectionTypeOracle.java#L14

Returns all properties, both inherited and declared, which would mean RedAccount re-lists the properties it inherited from the base Account. Does that look like it's the case?

[pbtura]

I wonder if the issue you're seeing is that properties: * does just not work well with inheritance...e.g. it could be that the oracle.getProperties:

https://github.com/stephenh/dtonator/blob/master/dtonator/src/main/java/com/bizo/dtonator/properties/ReflectionTypeOracle.java#L14

Returns all properties, both inherited and declared, which would mean RedAccount re-lists the properties it inherited from the base Account. Does that look like it's the case?

It looks that way. I switched back to the base code without any of my changes an ran it through the debugger. The 'getProperties()' method of ReflectionTypeOracle return all properties, including those inherited from the parent. When the config has explicit properties, the 'addPropertiesFromDomainObject()' method of DtoConfig will only include those properties, so the duplicates never get added to the Dto. If all properties are included using '*', then everything gets added and we end up with duplicates.

[stephenh]

Cool, thanks for validating my hunch. ...do you mind following up with a PR? :-) Thanks for digging in to this!

[pbtura]

Ok, I have a PR up that seems to solve most of the issue I was seeing. What I ended up doing was storing the 'declaringClass' of each getter/setter method in the Prop class. Then in the DtoConfig, it compares that stored class name with the domain type to see if the property should be included. There is probably a more elegant way to do that, but this is a start.