Add LC0054: Warning on database operations in TryFunctions #539
Conversation
StefanMaron
changed the title
There may be some ambiguity if all database operations should be avoided or only database write operations? If I understand de documentation correctly, only database write operations should be avoided. |
| @@ -95,6 +95,7 @@ Further note that you should have BcContainerHelper version 2.0.16 (or newer) in | |||
| |[LC0051](https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0051)|Do not assign a text to a target with smaller size.|Warning| | |||
| |[LC0052](https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0052)|The internal procedure is declared but never used.|Info| | |||
| |[LC0053](https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0053)|The internal procedure is only used in the object in which it is declared. Consider making the procedure local.|Info| | |||
| |[LC0054](https://github.com/StefanMaron/BusinessCentral.LinterCop/wiki/LC0054)|Do not call any database operations in TryFunctions.|Warning| | |||
There was a problem hiding this comment.
Should we specific mention write operations?
| @@ -591,4 +591,13 @@ | |||
| <data name="Rule0053InternalProcedureOnlyUsedInCurrentObjectAnalyzerTitle" xml:space="preserve"> | |||
| <value>The internal method is only used in the object in which it is declared.</value> | |||
| </data> | |||
| <data name="Rule0054DatabaseOperationInTryFunctionsDescription" xml:space="preserve"> | |||
| <value>Do not call write operations in try Functions</value> | |||
There was a problem hiding this comment.
Would it be better to make this the same as the documentation?
|
|
||
| private static readonly List<string> databaseInvocations = new List<string> | ||
| { | ||
| "insert", "delete", "modify", "modifyall", "rename", "addlink", "deletelink", "deletelinks", "commit" |
There was a problem hiding this comment.
If we need to take Rec.LockTable() into account, then we also should include Rec.ReadIsolation?
I do have a question about this do
Because changes made to the database by a try function are not rolled back, you should not include database write transactions within a try function
My first though is that a Rec.LockTable() doesn't created changes that could case issues when not rolled back. Do you have more insights or an example where this could cause an issue?
There was a problem hiding this comment.
I believe that below code should raise this warning
Rec.ReadIsolation := IsolationLevel::UpdLock;
Rec..FindFirst()
That is, IsolationLevel::UpdLock only causes a transaction if a read operation is called afterwards.
This can be even more complex, since the LockTable() or ReadIsolation could be called before the TryFunction is invoked, and a Find* operation is done inside the TryFunction. But I think that we could ignore that scenario, and focus on the 99.9% of cases 😅
There was a problem hiding this comment.
But maybe that kind of transaction is no problem for TryFunctions? 🤔
I need to set up an environment with the DisableWriteInsideTryFunctions set, to repro this...
|
I just love the idea with this rule! It is very challenging to find these issues during code review, since a |
|
Yes, but I did not find a way to find those type of scenarios unfortunately |
Ok, I see. But good to get started on this rule anyway! |
No description provided.