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

azure-http-specs, validate orphan model serializable #2318

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

weidongxu-microsoft
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft commented Mar 6, 2025

As discussed, we want to verify that an orphan model is by default serializable as JSON.

Since there were an existing test on orphan model, I just enhanced it and added an operation to verify the serialization.

@global.Azure.ClientGenerator.Core.convenientAPI(false)
@route("/orphanModelSerializable")
@put
op orphanModelSerializable(@body body: unknown): NoContentResponse;
Copy link
Member Author

@weidongxu-microsoft weidongxu-microsoft Mar 6, 2025

Choose a reason for hiding this comment

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

I use the unknown to avoid ref the OrphanModel.

Let me know any language that having difficulty to serialize a OrphanModel model into the request body of this op.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Mar 6, 2025

All changed packages have been documented.

  • @azure-tools/azure-http-specs
Show changes

@azure-tools/azure-http-specs - feature ✏️

Add orphanModelSerializable operation to verify the JSON serialization of an orphan model

Comment on lines 115 to 121
model OrphanModel {
@global.Azure.ClientGenerator.Core.clientName("modelName")
name: string;

@encodedName("application/json", "desc")
description: string;
}
Copy link
Member Author

@weidongxu-microsoft weidongxu-microsoft Mar 6, 2025

Choose a reason for hiding this comment

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

I've updated the model a little, to mix typespec name, client name, encoded name. And we expect SDK find the correct name for JSON serialization (it should be encoded name of application/json, if not available then typespec name).

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website

}

@doc("Not used anywhere, but access is override to public so still need to be generated and exported with serialization.")
@global.Azure.ClientGenerator.Core.usage(global.Azure.ClientGenerator.Core.Usage.input)
@global.Azure.ClientGenerator.Core.access(global.Azure.ClientGenerator.Core.Access.public)
model OrphanModel {
@global.Azure.ClientGenerator.Core.clientName("modelName")
Copy link
Member

Choose a reason for hiding this comment

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

i think we do need to have a way to indicate how to serialize an orphan model. it seems weird that i have an orphan model that one property has a client name.

Copy link
Member Author

@weidongxu-microsoft weidongxu-microsoft Mar 6, 2025

Choose a reason for hiding this comment

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

@clientName appears expected for me. This case maybe a bit uncommon that here rename is non-scoped, but I assume it be pretty common in scoped case that one language want one name, while other language want another.

And the eventgrid lib does exact this with @clientName on different SDKs.
https://github.com/Azure/azure-rest-api-specs/blob/main/specification/eventgrid/Azure.Messaging.EventGrid.SystemEvents/propertyNameOverrideCsharp.tsp
https://github.com/Azure/azure-rest-api-specs/blob/main/specification/eventgrid/Azure.Messaging.EventGrid.SystemEvents/propertyNameOverrideJava.tsp

But yes, we may need some way to say default to JSON, either argument to TCGC, option to emitter, or param to @service

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

Successfully merging this pull request may close these issues.

3 participants