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

[Serialization] Fix diverging rules for editor and runtime serialization of fields and properties #1875

Merged
merged 11 commits into from Oct 16, 2023

Conversation

Eideren
Copy link
Collaborator

@Eideren Eideren commented Oct 1, 2023

PR Details

Prevent serializing get-only properties without backing field.

The idea here is that they aren't safe enough to serialize by default since they are functions, getting the value may throw, and since they do not provide a setter to create a value if the getter ever throws, we decided to only include them if the user decorates them with a [DataMember]

Diverging rules between runtime and editor serialization

Yaml can now serialize internal fields if decorated with DataMember, reflecting how serialization and filtering works for properties.
Binary includes properties with internal getters, as long as they are decorated with DataMember as well.

Those two changes were required to ensure that both serializer behaved in the same way.

Related Issue

Didn't find any ?

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Pinging @IXLLEGACYIXL and @manio143 to take a look into this.

@Eideren Eideren marked this pull request as draft October 1, 2023 17:21
Copy link
Member

@manio143 manio143 left a comment

Choose a reason for hiding this comment

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

Note that anything in Stride.Core.Assets and Design wasn't running with binary serialization.

@Eideren Eideren marked this pull request as ready for review October 2, 2023 19:53
Comment on lines -1509 to -1523
[Fact]
public void TestImplicitMemberType()
{
var settings = new SerializerSettings() {LimitPrimitiveFlowSequence = 0};

var text = @"!ClassWithImplicitMemberType
Test:
String: test
";

settings.RegisterTagMapping("ClassWithImplicitMemberType", typeof(ClassWithImplicitMemberType));
settings.RegisterTagMapping("ClassWithImplicitMemberTypeInner", typeof(ClassWithImplicitMemberTypeInner));
SerialRoundTrip(settings, text);
}

Copy link
Collaborator Author

@Eideren Eideren Oct 2, 2023

Choose a reason for hiding this comment

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

This test has been removed as omitting object type when they match the type at initialization but diverge from the member's return type has been deprecated in a previous pr.
You'll see more changes related to this in other tests.

Copy link
Member

@manio143 manio143 left a comment

Choose a reason for hiding this comment

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

I'm surprised there's so many places which use private setters and are serializable, nice work! - Being explicit about it seems better.

@Eideren Eideren marked this pull request as draft October 4, 2023 15:17
@Eideren Eideren marked this pull request as ready for review October 4, 2023 16:23
@Eideren
Copy link
Collaborator Author

Eideren commented Oct 4, 2023

There's a fair amount members in the engine that don't conform with the binary's rules, i.e.: a bunch of

[DataMember]
public float MyVal { get; private set; }

But if they did end up at runtime they would have been broken before this change anyway since binary never supported private or protected members. I've looked at a couple and they seem to be used for either the UpdateEngine or for editor-only serialization.

@IXLLEGACYIXL
Copy link
Collaborator

There's a fair amount members in the engine that don't conform with the binary's rules, i.e.: a bunch of

[DataMember]
public float MyVal { get; private set; }

But if they did end up at runtime they would have been broken before this change anyway since binary never supported private or protected members. I've looked at a couple and they seem to be used for either the UpdateEngine or for editor-only serialization.

this is an invalid case and is not allowed.. atleast it should be and my diagnostics count that as error

but i only found around 8 cases with invalid combinations

@Eideren
Copy link
Collaborator Author

Eideren commented Oct 4, 2023

Yes, as I said, those are only in our source and used only in the editor, right now yaml still allows them through to avoid breaking everything until we take care of them. If you have an easy way to detect them feel free to share, what I was using for internal doesn't scale to private/protected.

@IXLLEGACYIXL
Copy link
Collaborator

Yes, as I said, those are only in our source and used only in the editor, right now yaml still allows them through to avoid breaking everything until we take care of them. If you have an easy way to detect them feel free to share, what I was using for internal doesn't scale to private/protected.

https://gist.github.com/IXLLEGACYIXL/b311896965b67af082eafde723619b54
i cant find more , mybe they dont ref stride core in their packages?

@Eideren
Copy link
Collaborator Author

Eideren commented Oct 5, 2023

We've discussed a bit more on discord and ended up agreeing on a less restrictive rule, there's a document covering it over here.
My comment there #1875 (comment) wasn't very explicit in what I meant, in that, at the time I wanted to prevent any and all usage of { get; private/protected set; } from being serialized while @IXLLEGACYIXL / Joreyk thought this was specifically about value types not being assignable when using a private/protected setter.

Last PR fixed the issue Joreyk logged.

@Eideren
Copy link
Collaborator Author

Eideren commented Oct 7, 2023

Will merge this one once #1694 is merged to avoid more merge conflicts over there.
Might introduce conflicts on this one but we'll see.

@IXLLEGACYIXL
Copy link
Collaborator

i have one last thing.
in object descriptor is this code

            // Member is not displayed if there is a YamlIgnore attribute on it
            if (AttributeRegistry.GetAttribute<DataMemberIgnoreAttribute>(memberInfo) != null)
                return false;

the comment is wrong, probably its a leftover from yamldotnet migration like manio said in discord
could you maybe remove it/alter it?

@Eideren Eideren merged commit b9f19bb into stride3d:master Oct 16, 2023
11 of 14 checks passed
@Eideren Eideren deleted the serialization_fix branch October 16, 2023 22:28
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.

None yet

3 participants