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.

Sign up for GitHub

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

do not skip_serializing of read only schemas #317

Merged
merged 1 commit into from
Jun 29, 2021
Merged

do not skip_serializing of read only schemas #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
23 changes: 16 additions & 7 deletions services/autorust/codegen/examples/gen_mgmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ const OUTPUT_FOLDER: &str = "../mgmt";
const ONLY_SERVICES: &[&str] = &[
// "vmware",
// "network",
// "cosmos-db",
// "databricks",
// "marketplace",
];

const SKIP_SERVICES: &[&str] = &[
Expand All @@ -22,12 +25,13 @@ const SKIP_SERVICES: &[&str] = &[
"deviceprovisioningservices", // TODO #82 certificate_name used as parameter more than once
"dnc", // https://github.com/Azure/azure-rest-api-specs/pull/11578 two ControllerDetails types
"m365securityandcompliance", // can't find privateLinkServicesForO365ManagementActivityAPI.json
"mixedreality", // TODO #83 AccountKeyRegenerateRequest not generated
"netapp", // Ident "10minutely"
"network", // recursive types need to be boxed
"powerplatform", // https://github.com/Azure/azure-rest-api-specs/pull/11580 incorrect ref & duplicate Operations_List
"service-map", // Ident "Ref:machine"
"servicefabric", // https://github.com/Azure/azure-rest-api-specs/pull/11581 allOf mistakes and duplicate Operations_List
"marketplace",
"mixedreality", // TODO #83 AccountKeyRegenerateRequest not generated
"netapp", // Ident "10minutely"
"network", // recursive types need to be boxed
"powerplatform", // https://github.com/Azure/azure-rest-api-specs/pull/11580 incorrect ref & duplicate Operations_List
"service-map", // Ident "Ref:machine"
"servicefabric", // https://github.com/Azure/azure-rest-api-specs/pull/11581 allOf mistakes and duplicate Operations_List
"servicefabricmanagedclusters",
"synapse", // TODO #80 path parameters
"web", // TODO #81 DataType::File
Expand All @@ -44,6 +48,8 @@ const SKIP_SERVICE_TAGS: &[(&str, &str)] = &[
("compute", "package-2021-03-01-only"), // TODO #81 DataType::File
("consumption", "package-2019-11"), // ReservationRecommendationDetails_Get has a path and query param both named "scope"
("consumption", "package-2021-05"),
("cosmos-db", "package-2021-06"), // duplicate tag https://github.com/Azure/azure-rest-api-specs/issues/14996
("databricks", "package-2021-04-01-preview"), // duplicate tag https://github.com/Azure/azure-rest-api-specs/issues/14995
// datamigration, same error for all
// SchemaNotFound MigrateSqlServerSqlDbTask.json ValidationStatus, but may be buried
("datamigration", "package-2018-07-15-preview"),
Expand All @@ -52,7 +58,10 @@ const SKIP_SERVICE_TAGS: &[(&str, &str)] = &[
("datamigration", "package-2018-03-15-preview"),
("datamigration", "package-2017-11-15-preview"),
("mediaservices", "package-2019-05-preview"), // invalid unicode character of a dash instead of a hyphen https://github.com/Azure/azure-rest-api-specs/pull/11576
("marketplace", "package-composite-v1"),
("marketplace", "package-2020-01-01"),
("marketplace", "package-2020-12-01"),
("marketplace", "package-composite-v1"), // mixing versions
("marketplace", "package-composite-v2"), // mixing versions
// ("network", "package-2017-03-30-only"), // SchemaNotFound 2017-09-01/network.json SubResource
// ("network", "package-2020-11"), // recursive types need to be boxed
// ("network", "package-2021-02"), // recursive types need to be boxed
Expand Down
14 changes: 5 additions & 9 deletions services/autorust/codegen/src/codegen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,11 @@ impl CodeGen {
if &nm.to_string() != property_name {
serde_attrs.push(quote! { rename = #property_name });
}
if property.schema.read_only == Some(true) {
serde_attrs.push(quote! { skip_serializing });
Copy link
Member

Choose a reason for hiding this comment

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

Was this just because the read-only authoring couldn't necessarily be trusted? More so just curious. I think the change here looks safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is specific to the client side. I want to be able to do round-trip testing and I want to use the models from the server-side. One reason for read-only is so that the client doesn't send a bunch of the data back to the server. It is an optimization that isn't needed right now. Another design would be to generate a function that could clear the read only values, may be .clear_read_only().

Copy link
Member

Choose a reason for hiding this comment

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

It's a good optimization that I don't think we do in other language SDKs right now. I imagine the fear is that, for PATCH operations (could be implemented as a PUT as well), missing data handled incorrectly could result in data loss. @pakrym might have more insight into why we still send read-only data back, or if it's just an optimization we haven't yet done.

Copy link

@pakrym pakrym Jun 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@pakrym might have more insight into why we still send read-only data back

Do we? We shouldn't be.

https://github.com/Azure/autorest.csharp/blob/feature/v3/test/TestProjects/ModelShapes/Generated/Models/MixedModelWithReadonlyProperty.Serialization.cs#L16-L55

Copy link
Member

Choose a reason for hiding this comment

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

Good to know we don't for .NET. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

And, that said, if it hasn't been a detected problem yet this is probably a safe optimization. Ideally, if we did find a problem it should be a service-side fix since Azure has guidelines on patching resources.

} else {
if !is_required {
if is_vec {
serde_attrs.push(quote! { default, skip_serializing_if = "Vec::is_empty"});
} else {
serde_attrs.push(quote! { default, skip_serializing_if = "Option::is_none"});
}
if !is_required {
if is_vec {
serde_attrs.push(quote! { default, skip_serializing_if = "Vec::is_empty"});
} else {
serde_attrs.push(quote! { default, skip_serializing_if = "Option::is_none"});
}
}
let serde = if serde_attrs.len() > 0 {
Expand Down
6 changes: 3 additions & 3 deletions services/mgmt/addons/src/package_2017_05/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions services/mgmt/addons/src/package_2018_03/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions services/mgmt/adhybridhealthservice/src/package_2014_01/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 14 additions & 14 deletions services/mgmt/adp/src/package_2020_07_01_preview/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 17 additions & 17 deletions services/mgmt/adp/src/package_2021_02_01_preview/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions services/mgmt/advisor/src/package_2016_07_preview/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions services/mgmt/advisor/src/package_2017_03/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions services/mgmt/advisor/src/package_2017_04/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions services/mgmt/advisor/src/package_2020_01/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion services/mgmt/advisor/src/package_2020_07_preview/models.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 3 additions & 4 deletions services/mgmt/marketplace/Cargo.toml → services/mgmt/agrifood/Cargo.toml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading