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

DCS alignment to 4.8 with schema support #71752

Merged

Conversation

StephenMolloy
Copy link
Member

@StephenMolloy StephenMolloy commented Jul 7, 2022

Edit: Less drafty now. Just about ready to go. API reviews for the new System.Runtime.Serialization.Schema stuff (#72243) and the new API's exposed from DCS in the runtime to allow that package to do it's thing (#72242) have been approved.


Very, very drafty. But I need to get this out so you guys can get eyes on it. Several places I had questions or clarifications or just noticed something off... all those are marked with comments that have "TODO smolloy" so I can easily find them - and you can too.

I am probably not packaging the Schema project or including CodeDom in the correct way, but I figure project niggles can get sorted out later.

One issue that I still need to look deeper into is how we create DataContract's when importing schema. In 4.8 we just created the appropriate DataContract type with a default constructor and went on our merry way. In 5+ we are nullable notated and we don't have default constructors anymore and we expect every DC to have a non-null underlying type... which is what using default constructors avoided in 4.8. Undoing the non-nullability of UnderlyingType and all the code surrounding it was untenable starting out. So I create contracts with a dummy 'SchemaDefinedType' type. I still need to look into what problems that might cause and how to fix them.

Also note that we don't have any tests. I have not tested any of this schema code. The full suite of serialization tests as it exists in Core passes on my machine, but there is obviously a gaping coverage hole there wrt the schema work.

@StephenMolloy StephenMolloy added this to the 7.0.0 milestone Jul 7, 2022
@StephenMolloy StephenMolloy self-assigned this Jul 7, 2022
@StephenMolloy StephenMolloy added this to Open PRs in WCF Owned Areas via automation Jul 7, 2022
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/libraries/oob.proj Outdated Show resolved Hide resolved
@@ -518,15 +393,24 @@ internal bool RequiresMemberAccessForRead(SecurityException? securityException)
}
return true;
}
if (this.BaseContract != null && this.BaseContract.RequiresMemberAccessForRead(securityException))
if (BaseClassContract != null && BaseClassContract.RequiresMemberAccessForRead(securityException))
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Although out of scope in this change, there's a lot of cleanup that can be done around this area as a lot of it is to accommodate CAS. securityException should never be non-null as it's CAS that would throw the exception during RefEmit.

@mconnew
Copy link
Member

mconnew commented Aug 12, 2022

Nit, indentation


Refers to: src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema/CodeExporter.cs:305 in 7462bea. [](commit_id = 7462bea, deletion_comment = False)

@mconnew
Copy link
Member

mconnew commented Aug 12, 2022

This code previously had a constraint on dataContract.GenericInfo != null. Now it's always running. Are you relying on the earlier condition in the previous if block? Is it possible for dataContract.GenericInfo to be null and it to still be a generic type definition?


Refers to: src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema/CodeExporter.cs:575 in 7462bea. [](commit_id = 7462bea, deletion_comment = False)

@mconnew
Copy link
Member

mconnew commented Aug 12, 2022

I know it's equivalent, but I think the code would be easier to read if the comparison was >= 2 as there's an extra step when reading > 1 to parse that as "at least 2"


Refers to: src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema/CodeExporter.cs:664 in 7462bea. [](commit_id = 7462bea, deletion_comment = False)

@StephenMolloy
Copy link
Member Author

StephenMolloy commented Aug 12, 2022

@mconnew

This code previously had a constraint on dataContract.GenericInfo != null. Now it's always running. Are you relying on the earlier condition in the previous if block? Is it possible for dataContract.GenericInfo to be null and it to still be a generic type definition?

Refers to: src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema/CodeExporter.cs:575 in 7462bea. [](commit_id = 7462bea, deletion_comment = False)

You somehow got three comments in a row here that I can't resolve or reply to. Hrrmmm. Anyway, consider the nits before and after this one resolved.

As for this one, this was part of the decoupling referred to in this feedback item. The logic has been twisted to keep GenericInfo internal, and it is now handled in the call to DataContractSet.GetReferencedType(). In the end it should be equivallent. But this is the schema side of that untangling that you asked about in DCS.

@mconnew
Copy link
Member

mconnew commented Aug 12, 2022

Ack on the latest diff, all looks good to me.

@StephenMolloy StephenMolloy merged commit 802bbbd into dotnet:main Aug 13, 2022
WCF Owned Areas automation moved this from Open PRs to Completed Aug 13, 2022
Daniel-Svensson added a commit to Daniel-Svensson/runtime that referenced this pull request Aug 14, 2022
* Add missing Advance to prevent corruption of reader
* Correctly read decimal values
StephenMolloy pushed a commit that referenced this pull request Aug 15, 2022
* Read primitive types more efficient

* Allow stream to read more than minimum number of required bytes

* Remove pinning and pointer code from array reads

* Remove bounds checks from remaing array reads

* remove extra unchecked

* cleanup code

* Start adding tests

* Add tests for binary XmlDictionaryReader

* cleanup

* revert back to old byte read

* add test for arrays using "ref" enumeration

* add guid test

* Try to read while arrays from stream before processing

* Write guid arrays as memory on LittleEndian platforms

* Update tests to be independent of byte order

* FIx bugs introduced in #71752
* Add missing Advance to prevent corruption of reader
* Correctly read decimal values
@ghost ghost locked as resolved and limited conversation to collaborators Sep 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants