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

Feature/atr 557 dev check exceptions for serialization #462

Merged
merged 54 commits into from
Nov 14, 2022

Conversation

SENya1990
Copy link
Contributor

@SENya1990 SENya1990 commented Nov 7, 2022

Merge after ATR-139!!!

High level overview.
@EAndrosova this may be useful for the documentation of new diagnostics.

Diagnostics:

  • Implemented diagnostic PX1063 to check that exception-derived classes always declare serialization constructor.
  • Implemented diagnostic PX1064 to check that exception-derived classes declaring new serializable data always provide an override for the GetObjectData method.

By serializable data analysis understands any new data declared in the exception class that will be serialized by .Net serializers and Acumatica reflection based serializer:

  • New instance fields if they are not marked with System.NonSerializedAttribute attribute:
public int Field = 3;
  • New instance auto generated properties if they are not marked with System.NonSerializedAttribute attribute:
public int Property { get; set; } 

Some examples of what won't be considered serializable:

  • static fields and constants
  • static properties
  • calculated properties:
public int Property => 4;
  • properties with explicit backing field:
public int Property 
{
   get =>  _fieldFromBaseClass;
   set 
   {
       _fieldFromBaseClass = value;
   }
}
  • fields and properties marked as non serializable:
[field: NonSerialized]
public int Property { get; set; } 

Code Fixes:

  • Implemented code fix for PX1063 that will generate the missing serialization constructor for the exception class.
    The generated constructor will be private for sealed exception classes and protected for all other classes.
    The generated constructor will call the base serialization constructor.

If exception declares a new serializable data then the constructor will contain a call to the RestoreObjectProps static method of the PX.Common.ReflectionSerializer class. This class is responsible for Acumatica custom reflection-based serialization and deserialization of data. If there is no serializable data then the serialization constructor body will be empty.

  • Implemented code fix for PX1064 that will generate the missing GetObjectData override that will contain two statements:
    - First, a call to the GetObjectData static method of the PX.Common.ReflectionSerializer class
    - Second, a call to the base GetObjectData method:
    csharp base.GetObjectData(info, context);

  • In case of an old version of Acumatica there is no PX.Common.ReflectionSerializer class because it was added as a refactoring later. PX.Common.PXReflectionSerializer class will be used instead, the names of the methods are the same,

  • Code fixes will add all required missing using directives

Other Changes:

  • Added code generation utils to add missing using directives
  • Refactored DAC PK/FK related code fixes
  • Fixed a bug in the analysis of long operation started in graph view delegates when graph extension is passed to the StartOperation method (@EAndrosova , this is useful for release notes)
  • Added examples for new diagnostics to the demo solution
  • Added unit tests for diagnostics and code fixes
  • Updated Acumatica dlls used by demo solution and unit tests to 2022r200
  • Reworked unit tests infrastructure:
    - Added new metadata reference to netstandard.dll to the project dynamic generation
    - Added syntax parsing and compilation options
    - Added option to not use concurrent analysis when unit tests are debugged

…consistent with the majority of the messages
@dropsonic
Copy link
Collaborator

dropsonic commented Nov 7, 2022

@SENya1990 high-level overview is declared but missing

What is the point of removing dots from the end of each diagnostic's title?

<data name="PX1062FixFormatArg_Action" xml:space="preserve">
<value>action</value>
</data>
<data name="PX1063Title" xml:space="preserve">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EAndrosova please review diagnostic and code fix messages for PX1063 and PX1064

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to the following (too many nouns in a row + Acumatica is the company name so we should specify ERP)
The declaration of the exception class does not contain a serialization constructor. This will cause an incorrect deserialization of the exception data and will lead to runtime errors in Acumatica ERP.
The declaration of the exception class introduces new serializable fields but does not declare an override for the GetObjectData method. This will cause an incorrect deserialization of the exception data and will lead to runtime errors in Acumatica ERP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@SENya1990
Copy link
Contributor Author

SENya1990 commented Nov 7, 2022

@dropsonic , high level overview was missing because I was writing it. I added an important warning first and then started writing it.

The dots were removed because:

  • The style of the messages was not consistent. Most of the diagnostic and code fixes messages didn't have dots but some of them did.
  • Microsoft own diagnostic messages do not use dots:
    image

README.md Outdated
2. Add _PX.Data.dll_, _PX.Data.BQL.Fluent.dll_, _PX.Common.dll_, _PX.BulkInsert.dll_, _PX.Objects.dll_, and _PX.DbServices.dll_ (from Acumatica ERP 2019 R1 or higher) to the _lib_ folder.
Starting from Acumatica ERP 2020 R2, you also need to add _PX.Common.Std.dll_ to the _lib_ folder.
2. Add _PX.Data.dll_, _PX.Data.BQL.Fluent.dll_, _PX.Common.dll_, _PX.Common.Std.dll_, _PX.BulkInsert.dll_, _PX.Objects.dll_, and _PX.DbServices.dll_ (from Acumatica ERP 2022 R2 or higher) to the _lib_ folder.
You can use older versions of Acumatica dlls but there is no guarantee that Acuminator will work correctly. The current recommended version is 2022R2, no minor update.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EAndrosova please review the changes in the read.me file

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to the following:
You can use older versions of Acumatica ERP DLLs but there is no guarantee that Acuminator will work correctly. The current recommended version is Acumatica ERP 2022 R2 without minor updates.

If possible, specify RC or GA to make more clear what "no minor updates" mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

<data name="PX1062FixFormatArg_Action" xml:space="preserve">
<value>action</value>
</data>
<data name="PX1063Title" xml:space="preserve">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to the following (too many nouns in a row + Acumatica is the company name so we should specify ERP)
The declaration of the exception class does not contain a serialization constructor. This will cause an incorrect deserialization of the exception data and will lead to runtime errors in Acumatica ERP.
The declaration of the exception class introduces new serializable fields but does not declare an override for the GetObjectData method. This will cause an incorrect deserialization of the exception data and will lead to runtime errors in Acumatica ERP.

README.md Outdated
2. Add _PX.Data.dll_, _PX.Data.BQL.Fluent.dll_, _PX.Common.dll_, _PX.BulkInsert.dll_, _PX.Objects.dll_, and _PX.DbServices.dll_ (from Acumatica ERP 2019 R1 or higher) to the _lib_ folder.
Starting from Acumatica ERP 2020 R2, you also need to add _PX.Common.Std.dll_ to the _lib_ folder.
2. Add _PX.Data.dll_, _PX.Data.BQL.Fluent.dll_, _PX.Common.dll_, _PX.Common.Std.dll_, _PX.BulkInsert.dll_, _PX.Objects.dll_, and _PX.DbServices.dll_ (from Acumatica ERP 2022 R2 or higher) to the _lib_ folder.
You can use older versions of Acumatica dlls but there is no guarantee that Acuminator will work correctly. The current recommended version is 2022R2, no minor update.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to the following:
You can use older versions of Acumatica ERP DLLs but there is no guarantee that Acuminator will work correctly. The current recommended version is Acumatica ERP 2022 R2 without minor updates.

If possible, specify RC or GA to make more clear what "no minor updates" mean.

<value>The exception class declaration introduces new serializable fields but does not declare an override for the GetObjectData method. This will cause an incorrect deserialization of the exception data and will lead to runtime errors in Acumatica</value>
</data>
<data name="PX1064Fix" xml:space="preserve">
<value>Add an override for the GetObjectData method to the exception declaration</value>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EAndrosova please verify the code fix messages.

…entences to be consistent with MS code style for diagnostic messages with several sentences
@SENya1990
Copy link
Contributor Author

@dropsonic turns out MS style for diagnostic messages has different rules for dots in messages that consist of one sentences and messages composed from multiple sentences:

  • No dots for single sentences
  • Dots at the end of all sentences if there are two or more sentences

After discussion with @EAndrosova it was decided to be consistent with MS style, so I returned dots for all Acuminator messages consisting of several sentences and added a paragraph about these rules to the coding guidelines.

@SENya1990 SENya1990 merged commit 856d701 into dev Nov 14, 2022
@SENya1990 SENya1990 deleted the feature/ATR-557-dev-check-exceptions-for-serialization branch November 14, 2022 14:48
@SENya1990 SENya1990 linked an issue Nov 15, 2022 that may be closed by this pull request
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.

Add diagnostic to prevent (de)serialization errors
3 participants