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] More useful throw when missing parameterless ctor #2098

Merged
merged 5 commits into from
Jan 22, 2024

Conversation

Eideren
Copy link
Collaborator

@Eideren Eideren commented Jan 9, 2024

PR Details

Serialization throws with a random null ref exception when the type being deserialized does not have a parameterless constructor defined, this PR throws with a more specific exception. Users should be able to figure it out from there until @IXLLEGACYIXL work gets integrated.

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)
    ^ It would seem like this would be a breaking change, but all calling logic to that method actually expected it to never return null.

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@IXLLEGACYIXL
Copy link
Collaborator

IXLLEGACYIXL commented Jan 9, 2024

should i write a diagnostics analyzer for that case? as i cant handle parameter constrcutors either

@Eideren
Copy link
Collaborator Author

Eideren commented Jan 9, 2024

Yep, that would be best

@Kryptos-FR
Copy link
Member

Do we have unit tests covering these serialization cases? If so, they might need updating.

@IXLLEGACYIXL
Copy link
Collaborator

i have it halfway finished but i need polysharp for it, which is currently removed, lets see how it goes when the compilerservices are restored

@Eideren
Copy link
Collaborator Author

Eideren commented Jan 20, 2024

Added a simple test, it should be enough for this feature ... ?

}
catch (Exception ex)
{
Assert.True(ex.InnerException is DefaultObjectFactory.InstanceCreationException);
Copy link
Collaborator

Choose a reason for hiding this comment

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

cant you use here Assert.Throws?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I looked at it initially but didn't find an overload for inner exceptions ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Assert.That(ex.InnerException, Is.TypeOf<BadException>() ); mybe that one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to that one instead Assert.IsType<DefaultObjectFactory.InstanceCreationException>(ex.InnerException);, no API for .That()

@@ -119,11 +120,12 @@ public object Create(Type type)
}
}

return null;
throw new InstanceCreationException($"Failed to create instance of type '{type}', type does not implement a parameterless constructor.");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: a type has or doesn't have a parameterless constructor. It doesn't really implement it, the same way an interface or abstract method can be.

Suggested change
throw new InstanceCreationException($"Failed to create instance of type '{type}', type does not implement a parameterless constructor.");
throw new InstanceCreationException($"Failed to create instance of type '{type}', type does not have a parameterless constructor.");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@Eideren Eideren merged commit 80693e7 into stride3d:master Jan 22, 2024
13 checks passed
@Eideren Eideren deleted the serialization_ctor branch January 22, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants