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

Prevent ReadOnly/WriteOnly flag cascade on reusable object definitions #884

Merged
merged 2 commits into from
Jun 16, 2022

Conversation

jeskew
Copy link
Contributor

@jeskew jeskew commented Jun 13, 2022

Resolves #882

This PR updates the type generator to be aware of when it is generating a definition for a specific, named object (i.e., one that would be used via "$ref": "#/definitions/<type name>") and a "synthetic" object (one that is encountered when a resource input and output are defined separately). This allows the generator to know when it must flag properties as ReadOnly/WriteOnly, which should only happen with synthetic objects, as named object types may be reused in other contexts. Currently, if a shape is reused by multiple resources, it will have ReadOnly/WriteOnly usage flags applied per the resource defined earlier in the RP swagger model, and subsequent usages of the same type will retrieve the object definition from cache. If a shape is only present on the PUT request for one resource and only on the GET response for another, all of its properties will be defined as either ReadOnly or WriteOnly, depending on which resource is encountered first during type generation.

This functionality is important to #411, which will increase the amount of type reuse in this library (and, without a change like the one in this PR, will designate many of those reused types as consisting entirely of ReadOnly properties).

There are two changes to the type generator included in this PR that are meant to accomplish the goal described above:

  1. When the type generator has arrived at a property by walking a specific path down both the GET and PUT responses, the generator checks to see if both paths are pointing to the same named object. If they are not, a new object definition called <PUT path name>Or<GET path name> is created, and ReadOnly/WriteOnly property flags are applied as before.
    • For example, $.properties on Test.Rp1/testType1 in the integration test model, which is of type TestType1CreateOrUpdateProperties on the PUT request and of type TestType1Properties on the GET request. A new object definition named TestType1CreateOrUpdatePropertiesOrTestType1Properties is created, and the locationData property, which exists only on TestType1Properties, is flagged as ReadOnly.
  2. When the type generator is pointing to an object on only one of the GET or PUT schemata, it does not automatically flag the object's properties as ReadOnly or WriteOnly.
    • The properties of LocationData are not flagged as ReadOnly, even though they are first encountered by the type generator in the context of the Test.Rp1/testType1 resource. LocationData is also accepted as part of the request for the listFoos resource function, in which case it would not be composed of ReadOnly properties.

@jeskew jeskew requested a review from a team June 13, 2022 16:42
@codecov-commenter
Copy link

Codecov Report

Merging #884 (ee506d2) into main (6bc73cd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #884   +/-   ##
=======================================
  Coverage   95.57%   95.57%           
=======================================
  Files          18       18           
  Lines         294      294           
=======================================
  Hits          281      281           
  Misses         13       13           
Flag Coverage Δ
dotnet 95.57% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jeskew jeskew force-pushed the jeskew/update-named-type-property-flagging branch from ee506d2 to 04868f8 Compare June 14, 2022 14:02
@jeskew jeskew force-pushed the jeskew/update-named-type-property-flagging branch from 04868f8 to c9fb2fb Compare June 14, 2022 22:31
Copy link
Member

@anthony-c-martin anthony-c-martin left a comment

Choose a reason for hiding this comment

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

Looks great! @jeskew walked me through the impact to generated types yesterday, and the changes made sense.

@jeskew jeskew merged commit c7e0aae into main Jun 16, 2022
@jeskew jeskew deleted the jeskew/update-named-type-property-flagging branch June 16, 2022 15:09
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.

ReadOnly and WriteOnly flags should not cascade from objects to their properties
3 participants