Make ModelSerializer compatible with AOT#38010
Make ModelSerializer compatible with AOT#38010m-nash merged 13 commits intofeature/updateSerializerfrom
Conversation
m-nash
requested review from
JoshLove-msft,
KrzysztofCwalina,
annelo-msft,
christothes and
tg-msft
as code owners
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
|
@m-redding: Would you mind taking a look? |
| [System.AttributeUsageAttribute(System.AttributeTargets.Class)] | ||
| public sealed partial class DefaultSubClassAttribute : System.Attribute | ||
| { | ||
| public DefaultSubClassAttribute([System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] System.Type type) { } |
There was a problem hiding this comment.
Just to clarify - adding TrimmingAttributes.cs helps add this attribute here? And the guidance is avoiding differences between net5.0 and net6.0? It's fine that the others are different?
There was a problem hiding this comment.
@KrzysztofCwalina the public api surface still actually comes out different. Not sure if this is just a side effect of our api export tool or a real difference. Since you suggested we follow the NullableAttribute pattern can you take a look and see if this turned out the way you expected?
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
The DynamicallyAccessedMembers annotations look right to me.
sdk/core/Azure.Core/tests/public/Azure.Core.Tests.Public.csproj
Outdated
Show resolved
Hide resolved
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
LGTM - sans the api files not being updated correctly for the Deserialize<T> methods.
m-redding
left a comment
m-redding
left a comment
There was a problem hiding this comment.
Only other question I had was the one about the API differences, curious to see how that ends up
This could be related to the export-api.ps1 issue I opened a follow up issue on this. #38084 |
| public virtual System.Threading.Tasks.ValueTask<System.BinaryData> SerializeAsync(object? value, System.Type? inputType = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| } | ||
| [System.AttributeUsageAttribute(System.AttributeTargets.Class)] | ||
| public sealed partial class UnknownSubclassAttribute : System.Attribute |
There was a problem hiding this comment.
It feels weird to name this "UknownSubclass" when the subclass is actually known. That's what we are pointing to. So I guess I don't get what "Unknown" means in this context.
(Note: I'm just a bystander here, so take my feedback as just a random opinion.)
There was a problem hiding this comment.
This is for polymorphic forward compat. In v1 we have Animal := Cat | Dog which turns into abstract class Animal, class Cat : Animal, class Dog : Animal. In v2 they add class Fish : Animal. If someone creates a bunch of pets in v2 of the service, including fish, and our app using v1 of the service calls client.GetAllPets(); what do we return for those fish? We can't instantiate Animal because it's abstract. Instead we have a convention where we create an internal class UnknownAnimal : Animal that we can instantiate and return from v1 for resource kinds that never existed when v1 was built. That's what this attribute is intended to signal to our general purpose serializers in an AOT friendly way. We of course debated making Animal not abstract as well, but we really like the indicator to customers they need to write forward compat friendly code (i.e., have a reasonable default/fall through case).
It's fair that UnknownSubclass / UnknownAnimal still might not be a super clear indicator a time traveler just screwed with your global variables cloud resources. I'm not sure that FutureSubclass, ForwardCompatSubclass, UnknownResourceClass, etc., are necessarily better. Most customers won't care about the attribute itself though, other than seeing it's a thing in docs.
The internal class UnknownFoos are really where we should invest (i.e., a DebuggerDisplay or maybe ToString that clearly explains the discriminator wasn't known to the library as of Max(ServiceVersion)). @m-nash - We should also make sure the ref docs for UnknownSubclass have a helpful explanation for anyone who clicks on it from a random base class page since the attribute is public.
* Update serializer (#35735) * wip * wip * wip * wip * wip * Update serializer (#35741) * [Azure.Core] add List property tests and update ReadOnly Test (#35904) * [Azure.Core] Update SerializableOptions.cs (#35947) * [Azure.Core] add test helper for Models (#35968) * [Azure.Core] Update IJsonSerializable with non Try methods (#36164) * Add scaffolding for readme (#36342) * add scaffolding for readme * fix type on class name * one more typo :-( * make tests sync * Add use cases for explicit casts (#36391) * add use cases for explicit casts * remove extra empty line * add custom expected value for net462 * [Azure.Core] Update Serializer (#36376) * Add use cases for stj (#36399) * add use cases for stj * address feedback * add some clarification to readme * [Azure.Core] Add public static deserializer (#36402) * wip * wip * wip * Update StaticDeserializerTests.cs * Update StaticDeserializerTests.cs * Update StaticDeserializerTests.cs * Add model serializer example (#36418) * add use cases for stj * address feedback * add some clarification to readme * add model serializer example * move serialization classes into azure.core.serialization * update after namespace move * [Azure.Core] Add NewtonSoft Json example (#36453) * Update SerializableOptions.cs * wip * wip * update * wip * wip * add generic modeljsonconverter (#36854) * [Azure.Core] Add Newton soft BYOM case (#36794) * wip * Update ModelSerializer.cs * wip * Update ReadOnlyPropertyTests.cs * wip * Update SerializableOptions.cs * Update SerializableOptions.cs * wip * wip * Update SerializationSamples.cs * regen api after merge * Demo of what it would look like for public surface (#36857) * demo of what it would look like for public surface * wip * update tests and more renames * name tweaks * update api * add nullable * adjust public api surface and move test cases to public project to avoid internalsvisibleto * integrate feature/updateSerializer * rename IModel to IModelSerializable since it now has the serialization methods on it * share deserialization code between converter and modelserializer * [Azure.Core] Add XmlSerializer (#37155) * wip * wip * Update Serialization.md * Update ModelXmlTests.cs * wip * wip * update combined interface example --------- Co-authored-by: m-nash <prognash@gmail.com> * add examples for combined interface (#37245) * regen api after merge * Update to v1 api surface (#37276) * update to v1 api surface * update api after merge * wip (#37351) * wip (#37544) * [Azure.Core] Update options to use Format string (#37467) * wip * wip * Update UsingJsonSerializerTests.cs * wip * Update Serialization.md * wip * wip * wip * wip * wip * wip * wip * wip * Update ModelSerializerOptions.cs * wip * Update ModelSerializerOptions.cs * Update ModelSerializerOptions.cs * Update format to have J and W and implement json always on xml models (#37815) * update format to have J and W and implement json always on xml models * update api * Perf checks (#37349) * wip * update benchmark to use a real model from compute * update api * add multi buffer option and benchmarks for larger object * minor fixes on perf * perf updates and split the xml / json interfaces so we can take Utf8JsonWriter * add delayed RequestContent to improve implicit cast perf * fixes after merge * add utf8jsonreader deserialize * update for the rest of the test models with jsonreader * update api * fix typo * upload bimodal repro * return values from benchmarks * updates after merge * update modelserializeroptions to a struct * wip * update to split request content and sequence writer * update api * remove static azure default options since new is zero alloc * update api * add non generic overloads in modelserializer * Revert "update api" This reverts commit 34c0f6b. * Revert "remove static azure default options since new is zero alloc" This reverts commit 5f7bcfa. * wip * updates after merge * update api * add memory diag by default for all benchmarks * use fixed settings to run benchmarks faster * add package needed to use ETW profiler in benchmark * change dictionary to resolver factory * update options back to class * update to make interface generic * remove two way implicit * add XElement api on public interface * update api * swap to struct for options * remove unused shared source * Hide get sequence until we deal with use-after-free (#37893) * hide getsequence until we deal with use-after-free * update api * remove broken inheritdocs * Testing public test project failure in ci (#37894) * testing public test project failure in ci * mark the project as test project * add public test to split list * set support project to false to run tests in ci * add failing case after merge * remove failing test * Address api review comments and add more xml docs (#37916) * address api review feedback * switch options to class and add a fozen static reference * update api * rename WriteTo -> CopyTo * update snippets * update request content create to take an imodelserializable (#37939) * Mnash request conent i model serializable (#37941) * update request content create to take an imodelserializable * make sequenceWriter internal * update reflection case for modelserializer (#37945) * Address archboard feedback (#37949) * move convert to binary data onto modelserializer * validate error scenarios for modelserializer * rename default wire options * add overload to converter ctor * update for format overloads and docs * update format docs * add tests for more validation around multiple types in a single json serializer instance * Remove xml interface (#37967) * remove IModelXmlSerializable * update api * Address feedback from #35946 (#37971) * remove IModelXmlSerializable * update api * address initial feedback * remove reference to utf8jsonreader helpers * update debug assert * Add some more testing around SequenceWriter (#37973) * add some tests * more tests * wip * add benchmarks for RequestContent * update request content to use sequence writer access class * add write lock to serialization * remove empty file * harden flaky test * add singletons for known format types (#38007) * Move SequenceWriter to private class (#38013) * add additional feedback on sequenceWriterAccess * move inner private class to another file * delete Readme and associated files (#38064) * Add tests for model serialization (#38042) * re-organize common test code and add initial tests for ModelWriter * missed files * update perf proj to use common testdata * group model serializer tests * update api * wip * separate read lock * make cast succeed on null (#38105) * Make ModelSerializer compatible with AOT (#38010) * add new attribute to deterministically find the default subclass * update test since message changed * address comments * add aot attributes up the call chain * remove un-needed attribute * add unreferenced code attribute to ModelJsonConverter * remove attribute from jsonconverter overload * conditionally add the unreferencedcode attribute to modeljsonconverter class * address comments * rename unknown subclass attribute * Update sdk/core/Azure.Core/src/Serialization/IModelJsonSerializable.cs Co-authored-by: Anne Thompson <annelo@microsoft.com> * Update sdk/core/Azure.Core/src/Serialization/IModelSerializable.cs Co-authored-by: Anne Thompson <annelo@microsoft.com> * Update sdk/core/Azure.Core/src/Serialization/IModelSerializable.cs Co-authored-by: Anne Thompson <annelo@microsoft.com> * address exception feedback (#38120) * update helper so its non boxing and add struct test cases (#38125) * Address some final feedback (#38130) * hide model writer * rename abstract hierarchy deserializer * update wire format docs * Minor cleanup (#38137) * address feedback * add checks around stream with long length * change from notsupported to format exception (#38147) * address feedback (#38150) * update changelog --------- Co-authored-by: m-nash <64171366+m-nash@users.noreply.github.com> Co-authored-by: m-nash <prognash@gmail.com> Co-authored-by: Anne Thompson <annelo@microsoft.com>
6 participants
Address this comment #35946 (comment)
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.