Azure / autorest.java

Extension for AutoRest (https://github.com/Azure/autorest) that generates Java code
MIT License
32 stars 80 forks source link

mgmt azure-json, `fromJson` cannot access read-only property from `Resource` #2740

Open XiaofeiCao opened 2 months ago

XiaofeiCao commented 2 months ago

Issue

Current generated code:

if ("id".equals(fieldName)) {
    deserializedDedicatedHostGroupInner.withId(reader.getString());
} else if ("name".equals(fieldName)) {
    deserializedDedicatedHostGroupInner.withName(reader.getString());
} else if ("type".equals(fieldName)) {
    deserializedDedicatedHostGroupInner.withType(reader.getString());
}

Proposals:

Solution 1: Add protected setters in Resource/ProxyResource

Pros

  1. Seemingly the most simple to implement. In fact, current codegen's working like magic as it assumes setter methods on parent classes. There's not much(if not none) codegen work involved.

    Cons

  2. Protected setter methods are exposed though all Resource/ProxyResource's subclasses.

    Solution 2: Add protected constructors in Resource/ProxyResource

    Pros

  3. Exposed less amount(1 vs 3) protected methods than the setter
  4. Preserves the "read-only" nature of these properties, compared to "setter"

    Cons

  5. Protected constructor is exposed any way.
  6. mgmt doesn't have constructors with parameters for now
  7. Way more things to implement than protected setters.

    Solution 3: Shadowing id, name, etc... in child classes, similar to https://github.com/Azure/autorest.java/pull/2639

    Pros

  8. No unnecessary exposed methods/properties
  9. No core dev work
  10. Potentially consistent with general case: https://github.com/Azure/autorest.java/issues/2741

    Cons

  11. Duplicated shadowing properties across all inheritance hierarchy, making subclasses potentially cumbersome.
  12. More dev work in codegen than protected setter approach.

    Solution 4: Move ProxyResourceAccessor out of implementation package

    Pros

  13. No exposed protected methods/constructors

    Cons

  14. Violates the Accessor Helper design, where the helper should be in implementation.

    Access helpers are defined in implementation packages and should be defined in the package implementation.accesshelpers

  15. Similar as above, instead of exposing protected methods, it exposes unnecessary public Accessor Helper class.

Personal preference

From API's perspective, I prefer solution 3, which is the shadowing approach. It doesn't expose unnecessary APIs. Additionally, this may be consistent with the general case https://github.com/Azure/autorest.java/issues/2741, e.g. if the property is from parent and no public setter available, we shadow/duplicate it in child classes. Haven't dived deep into each of the implementation. I assume all the solutions except solution 1(protected setters) involve getting some information of the base class(Resource/ProxyResource) to determine property's access pattern.

weidongxu-microsoft commented 1 month ago

My vote probably on 3 as well. But please also initiate the discussion in channel.