Skip to content
This repository was archived by the owner on Jul 6, 2026. It is now read-only.

client, add shadow variable for discriminator property#2639

Merged
weidongxu-microsoft merged 14 commits into
Azure:mainfrom
weidongxu-microsoft:client_getKind
Mar 26, 2024
Merged

client, add shadow variable for discriminator property#2639
weidongxu-microsoft merged 14 commits into
Azure:mainfrom
weidongxu-microsoft:client_getKind

Conversation

@weidongxu-microsoft

@weidongxu-microsoft weidongxu-microsoft commented Mar 25, 2024

Copy link
Copy Markdown
Member

another fix on #2574

Remove setKind method in superclass.
Shadow kind variable in subclass.
Override getKind method in subclass.

code change https://github.com/Azure/autorest.java/pull/2639/files/e9c1c0dbad94325dacb299fc8f29a1d6c892fd7b..db632bbb53ab4612517996b0298ea42f44824de1

test like this (for both azure-json and Jackson) e9c1c0d

Comment on lines +139 to +143
if (!property.isPolymorphicDiscriminator()) {
superPropertyConsumer(property, superRequiredProperties, superConstructorProperties,
superReadOnlyProperties, superSetterProperties, settings);
hasRequiredProperties |= property.isRequired();
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Treat polymorphic discriminator property as a property in current model.

? JavaVisibility.Private
: JavaVisibility.Public;

if (!property.isPolymorphicDiscriminator() || modelDefinesProperty(model, property)) {

@weidongxu-microsoft weidongxu-microsoft Mar 25, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here code change is just removing the condition (since we no longer skip the discriminator property).

* The @data.kind property.
*/
@Generated
private String type = "bytes";

@weidongxu-microsoft weidongxu-microsoft Mar 25, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My origin attempt is to avoid this String type class variable, and directly provide

    @Generated
    @Override
    public String getType() {
        return "bytes";
    }

(there is a corner case on behavior difference between the 2, on a case of user directly do BinaryData("<invalid-json>").toObject(BytesData.class) with a JSON of unknown type)

But reverted to current shadow variable.
Reason is fromJson have code on either deserializedBytesData.setType(..) or deserializedBytesData.type = .., and I don't want to disrupt that part of code.

*/
@Generated
@Override
public String getType() {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume, in subclass, adding/removing the "override" of getType() in exact same method signature won't be breaking changes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://revapi.org/revapi-java/0.28.1/differences.html#java.method.movedToSuperClass

"Method Moved To Superclass" is "equivalent". So I think this won't be counted as break.

@weidongxu-microsoft weidongxu-microsoft merged commit 053fefd into Azure:main Mar 26, 2024
@weidongxu-microsoft weidongxu-microsoft deleted the client_getKind branch March 26, 2024 03:06
public JsonWriter toJson(JsonWriter jsonWriter) throws IOException {
jsonWriter.writeStartObject();
jsonWriter.writeIntField("age", getAge());
jsonWriter.writeStringField("kind", getKind());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just notice serialziation and deserialization part, wonder why this get removed?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants