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

Cleanup serialization #1469

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

ngbrown
Copy link
Contributor

@ngbrown ngbrown commented Dec 3, 2017

This is a split-off of what was #1425.

This focuses only on the clean up of attributes and overloads. I used some Roslyn Analyzers that I wrote based on:

There are changes to visibility in the public API.

Possible breaking changes:

  • Serialization of some exceptions has changed.
  • StatefulPersistenceContext and ProxyObjectReference are now sealed.
  • De-serialization constructors which were public are now protected. This impacts following classes: Configuration, DuplicateMappingException, ADOConnectionException, ConstraintViolationException, DataException, GenericADOException, LockAcquisitionException, SQLGrammarException, InvalidProxyTypeException.
  • Classes marked with Serializable attribute but which were not actually serializable have no more the attribute. This impacts following classes: Mappings, NativeSQLQueryPlan, AbstractEvent and all its descendants, EntityMetamodel, Property and all its descendants.
  • ISerializable classes lacking a de-serialization constructor have now one. It should be called by descendant classes own de-serialization constructor. This impacts following classes: CascadeStyle, SqlParseException, DetailedSemanticException, InvalidPathException, SemanticException, QueryExecutionRequestException, ParserException.

get { return targetType; }
get
{
if (targetType != null || targetTypeAssemblyQualifiedName == null) return targetType;
Copy link
Contributor Author

@ngbrown ngbrown Dec 3, 2017

Choose a reason for hiding this comment

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

From #1425 (comment):

Why not restoring it in the deserialization constructor? (It will allow keeping it readonly by the way. The additional field could be avoided too.)

Copy link
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

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

LGTM for 6.0. I have edited the first comment for listing in it possible breaking changes, in order to help writing the release notes.

@@ -325,7 +325,7 @@ public static System.Type ClassForName(string name)
/// the method try to find the System.Type scanning all Assemblies of the <see cref="AppDomain.CurrentDomain"/>.
/// </remarks>
/// <exception cref="TypeLoadException">If no System.Type was found for <paramref name="classFullName"/>.</exception>
public static System.Type ClassForFullName(string classFullName)
public static System.Type ClassForFullName(string classFullName)
Copy link
Member

Choose a reason for hiding this comment

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

Reformatting this file is quite an unrelated change but well, let it be. (Checked with ?w=1, no unexpected changes.)

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.

None yet

3 participants