Fixing Preview Posting: Part 2

In my two last posts, I laid out the technical design of the Preview Posting feature, and then showed how to simplify its design to get around the unwanted behavior of TryFunction.

One thing is obvious from these posts: Preview Posting wants to simulate posting of a document (or journal) to show what the results would be, but without leaving any actual trace in the database. In database lingo – we want the transaction to be rolled back.

Rolling back is no brainer. Simply throw an error, or do a silent abort, Except if there is COMMIT. In that case, no rolling back helps.

I have announced earlier that my today’s post will be about how to nest transactions in C/AL so that an inner COMMIT has no effect on the transaction as a whole, which can still be successfully rolled back.

That’s what this post is about.

Nested transactions are nothing special for SQL. Take a look at this T-SQL batch:

image

When you run it, you see this:

image

We started a transaction, showed that there is data in the table, then started an inner transaction, then deleted all data, committed the inner transaction to the database, showed that there is no data in the table, then rolled back, and then showed that data is still untouched in the table.

In SQL Server, you can nest transactions down to 32 levels, and yet NAV – which runs on top of this powerful engine – seemingly cannot nest transactions at all. In C/AL everything is always one level deep, and when you commit, you commit all, and when you rollback, you rollback all.

Or, at least, it used to be that way all the way up to NAV 2013. Starting with NAV 2013, you can nest transactions, but you need to pull a clever trick: you need to call into the testability framework.

Now, I know that testability framework is not intended to be used in scenarios that I am about to describe, and it’s also not widely familiar to developers, so give me little latitude here.

Since version 2013, NAV’s testability framework includes an interesting new property on the test runner codeunits: TestIsolation. It comes with three options: None (default), Function, Codeunit.

With the None option, each test function runs inside as an isolated transaction which is committed at the end of the test. If the test succeeded, any data it wrote is persisted in the database. If there was a COMMIT anywhere in the test (and provided you didn’t change the TransactionModel property of the test function) then data up to that point is committed, and a new transaction is started. If the test fails after that point, data that was committed, is persisted. It behaves exactly as you would expect C/AL code to behave.

However, with Function and Codeunit options, each test function, or codeunit, respectively, are wrapped into an outer transaction, which is rolled back at the end of the test function (or test codeunit) execution. Regardless of whether a test succeeded or failed, the transaction is rolled back, and no data is persisted. Even if there was one or more COMMITs anywhere inside the code executed by the test, everything is rolled back. This is possible because to enable this feature, NAV makes full use of SQL Server transaction nesting capability.

Let me now put a big, fat, stinking disclaimer here: I am not recommending you to use this approach – I am simply using this example to show that concept of nested transactions is not foreign to C/AL and that you can easily put it to practical use if you need it. The testability framework is designed to be used solely for the purposes of unit testing and was not intended by Microsoft to be used in production. All code it executes will still happily run in production, but Microsoft will always officially discourage such use, and will never want to provide any support for anything that results out of its use.

So now that you know that you shouldn’t try this at home, let’s redesign the Preview Posting feature so that it takes advantage of transaction nesting in C/AL through testability framework.

This is what we’ll need:

  • A test codeunit that runs codeunit 80
  • A test runner codeunit that runs the test codeunit
  • Commenting a line of code in codeunit 19
  • Rewriting the Preview function in codeunit codeunit 81

Let’s get started.

First, create a new codeunit, set its Subtype to Test, and add the following code:

image

It uses one global variable of type Record 36 and a function to set this variable from the test runner codeunit.

The test function is a simplified version of the part of the Code function in codeunit 81, except it also calls the  Finish function in codeunit 19 – something that normally happens a bit later in codeunit 19. We need it here, because at this point data is still in the database, and not rolled back by the test runner. Also, we don’t really need the PreviewMode, because we don’t care if any COMMIT is called, but without it – even though everything is correctly rolled back at the end – there is a distinct peculiar behavior if preview mode is not set, that I’ll explained later.

Now, create another codeunit, set its Subtype to TestRunner, and add the following code:

image

It has one global variable for Record 36. You first must set this variable through the call to SetSalesHeader.

When you run this codeunit, it first sets the sales header record on the test codeunit, and then runs the test codeunit.

OnBeforeTestRun always returns TRUE because you want to run all the tests. This trigger will run twice: first time for the test codeunit, and the second time for the SalesPostPreview function.

In OnAfterTestRun we simply throw any error that happened during posting. If there was an error, the user must see it.

Make sure to set the TestIsolation to either Function or Codeunit – it doesn’t really matter, as long as it is not None.

Now, let’s go to codeunit 19, and make a minor change:

image

The only purpose of this CONSISTENT call was to prevent accidental COMMITs. Since we don’t care if there are any COMMITs, we don’t need this.

And finally, codeunit 81, and it’s Preview function. I won’t comment and then insert, because I am fully rewriting it, so the following code example doesn’t follow commenting best practices:

image

In here, we start the preview, then set the sales header on the test runner and then run the test runner. Test runner will run the posting wrapped in an outer transaction, which it will duly roll back at the end. Finally we show the entries collected inside the test codeunit.

And that’s all. When you run Preview Posting, you get the preview of the posting results, and no trace of any changes in the database.

(One issue: when you comment out the SetPreviewMode function, then the sales header record is deleted as a part of the posting procedure. Even though it is rolled back at the end, NAV will close the sales order page, and hide the preview-posted order from the order list. Pressing F5 will show it again. Since this is undesirable for preview posting, I kept the preview mode in place.)

Now that you have seen how it works and why, I’ll once again re-state that this is not really how Preview Posting should have been originally designed. The reason why I showed this is to:

  1. Show that Microsoft has already modified the C/AL behavior to allow nested transactions when they redesigned the testability framework for version 2013. This change in behavior was justified, and was implemented properly, without dubious (at best!) design choices (as in the case of TryFunction).
  2. Propose a different implementation of TryFunction logic and a bit of improved transaction handling in C/AL.

Now that we have seen this example of Preview Posting implemented in three different ways – none of which is perfect – we must agree that both of these claims are true:

  • There is justified business need for better transaction control in C/AL; and
  • The NAV runtime already possesses all of the functionality needed to properly make full use of the database stack and nested transactions.

I believe you already agree with me that TryFunction is not implemented the right way and that it’s behavior of not rolling back data and allowing you to commit data after an error is plain wrong. There are several ways of properly implementing it, and we can argue about those, but my suggestion of the implementation of the TryFunction feature would be the following:

  • When TryFunction is called, NAV always starts an inner transaction.
  • If there is a database error inside the TryFunction, the whole transaction is marked as uncommittable.
  • If there is an explicit call to COMMIT inside the TryFunction, it is ignored. Maybe SAVE TRANSACTION is invoked on SQL Server, but without properly brainstorming it, I believe simply ignoring it is okay.
  • At the end of TryFunction, if there was no database error, NAV commits the inner transaction.

This would make a lot of sense.

First – if there as a database error, it should not be possible to commit the transaction or parts of the transaction. That premise is at the heart of transactional systems, and NAV should not reinvent any (wrong) wheels here. The concept of uncommittable transactions already exists in NAV – we can explicitly mark transaction as uncommittable through CONSISTENT(FALSE). Here, it would be the runtime that marks the transaction as uncommittable when necessary.

Second – there is no need for the dirty CONSISTENT trick or any other trick for that matter. CONSISTENT(FALSE) only makes sure that no COMMIT is hit inside a try function, because that is really disastrous (with the current behavior of not rolling back stuff). With inner transactions and concept of uncommittability, this problem is solved.

On top of that I propose to introduce two more C/AL functions:

  • ROLLBACK. Do I really need to elaborate? We may want to rollback data and then continue the execution. We already can do that with ASSERTERROR ERROR(‘’) trick which looks mighty confusing. Why not just replacing it with a proper keyword?
  • ISCOMMITTABLE. This function would tell us whether we are in an okay state. If it’s false, it would mean that some TryFunction failed at some point earlier.

These changes would take care of all the problems we currently have with TryFunction.

Now, I must admit that I didn’t analyze all the TryFunctions Microsoft put into standard application code. There might be a legitimate reason for a TryFunction not to rollback data, and maybe there is a legitimate reason to allow persisting data to the database even in case of error. If this is really true, then I would propose some more behavior changes:

  • Limit the uncomittability state for transactions only to database errors that happen inside TryFunctions. If there is any other kind of error inside TryFunction, then let the user decide what to do. For example, a TryFunction should not prevent a database transaction from committing if there was, for example, a file read error on a .NET interop variable.
  • Introduce savepoints and a new C/AL function: SAVETRANSACTION. This function would simply call the SAVE TRANSACTION in T-SQL. Then, if there is an error inside a TryFunction, it would only prevent committing stuff to the last savepoint.

However, these latest two suggestions are an overkill and not really necessary. My firm position is that a database error is a database error and after a database error no committing should be possible and if you need some data to persist, and some not to persist, then you structure your code differently (e.g. use COMMIT before TryFunction) or you should pull some real tricks to make it work, not the other way around (that being now: we have to pull crazy tricks to make it behave the way it always has, and it always should).

With this I rest my case about TryFunctions and transaction integrity. I hope I have proven my case beyond any reasonable doubt, and I hope there is still time for Microsoft to reconsider the existing behavior.

16 thoughts on “Fixing Preview Posting: Part 2”

  1. Hi Vjeko,

    I loved reading your series on the new Try-function and all (problematic) implications of it!

    I would like to share one piece of information about that approach to use a TestRunner codeunit. The other day I was using TestRunner-Codeunits to simulate a Load-Tests in kind of a combination of the test toolkit and the old benchmark toolkit. Everything worked very smoothly with the exception of occasional deadlocks that may happen in any load test scenario. To make a long story short: background sessions that called my TestRunner-codeunits would stop with an error if a deadlocks occurs even if I tried to handle any error in the inner test-codeunit (IF CODEUNIT.RUN(TestCodeunitNo) THEN …). Any other “inner” error was caught successfully.

    I made the tests in NAV 2015 CU 10, I will test NAV 2016 if I find time.

    The exception in the event viewer of the NST looked like:
    Message: An unexpected error occurred while ending the transaction. The transaction was rolled back.
    StackTrace:
    at Microsoft.Dynamics.Nav.Runtime.NavSqlConnection.RollbackTransactionSavepoint(String savepoint)
    at Microsoft.Dynamics.Nav.Runtime.NavSqlConnectionTestScope.TryTransactionEnd(NavSqlEndTransactionType endType)
    at Microsoft.Dynamics.Nav.Runtime.SqlTableDataProvider.EndTransaction(Boolean commit)
    at Microsoft.Dynamics.Nav.Runtime.TransactionManager.IssueEndTransaction(Boolean commit)

    The only solution that I found was to remove the TestIsolation-property since I am doing the load test in a dedicated load test environment. Have you ever experienced something similar?

    Best regards
    Julian

  2. Well, this is a good one! using testcodeunits functionality to get some user functionality. Nice.
    I think the tryfunction has been created to prevent some extra lines of code. Just like they did with ISTEMPORARY and RECORDID. Aren’t we all lazy programmers?

    Just take a look at the changes made in codeunit 1290 and how they are used in 1289.

    The function SendRequestToWebService is now a tryfunction, whereas in NAV2015 they used the “if codeunit.run” functionality.

    As a result:(old code from codeunit 1289)

    — NAV2015—-
    {…}
    COMMIT;

    IF WebServiceRequestMgt.RUN THEN BEGIN
    WebServiceRequestMgt.GetResponseContent(ResponseInStream);

    — end of NAV2015 code —

    which has changed to:

    — NAV2016 —
    {…}
    IF SOAPWebServiceRequestMgt.SendRequestToWebService THEN BEGIN
    SOAPWebServiceRequestMgt.GetResponseContent(ResponseInStream);
    — end of NAV2016 —

    So no need to commit before doing if codeunit.run anymore.
    Which makes sense, if doing webservice calls.

    1. Of course they introduced TryFunction to replace the IF CODEUNIT.RUN construct. However – they implemented it wrong way around. As I suggest – for non-database errors, TryFunction should be okay to not rollback anything. Well, I said I rest my case, and I do 🙂

  3. As it was already said: “Amen!!!”. And I just wanted to say thanks for spending all the time on this. Plus honestly this was fun to read (go geeks everywhere). I don’t recall the last time I had to sit and think a bit about something related to NAV coding … So thanks for giving some much needed exercise to my last remaining neurons … And hopefully MS is listening. If they change it like you asked then I propose the Try function should be renamed to Vjeko 🙂

  4. First of all — brilliant post/series. Well written and thought provoking. I’m assuming you made the same suggestions to MS during the 2016 beta and was ignored? You don’t have to answer that, but if that is the case it makes me really sad. I think the MS team have done an amazing job with NAV 2016, but I wish they would listen more. Can you please go and work for MS in a new position called “VP of NAV Technical Architecture and Avoiding Stupid Design Decisions.” Thanks again for the dedication you put into the NAV community.

    1. Dave, thanks for the comment. I hate myself for not having tested this thing properly during beta. It was there, I gave it a ride when it first appeared, but didn’t really test it properly – in all honesty, I didn’t expect it would behave the way it ended. Then, another MVP (Soren) brought the whole concept to my attention when we were sipping drinks at the conference in Mannheim – he commented in disbelief what he read in the help file. I am really really sorry for not having found enough time to properly focus on this feature earlier, and that’s all. I am pretty sure that if I had noticed this before, I’d energize the whole MVP group to be loud enough for Microsoft to reconsider. And I still believe they will reconsider. Thanks for your support, I really mean it!

    2. By the way – Microsoft is mostly listening to comments from ACE/TAP program testers. In this instance, they can’t be blamed for not listening.

  5. Vjeko, good post. You read my mind. I was thinking about the testrunner too.

    I’ve followed the feature when it was first introduced in the TAP/ACE/SME/Whatever programm. The first implementation in early CTP builds used the ASSERTERROR function which was programmed on the page.

    I blame myself for not paying attention. I was happy when they moved the code from a page to a codeunit. I never caught the change to the Try Function though.

    Let’s see where this ends. We’ll have a meeting in Orlando.

    Again, good job and thanks for taking the time.

  6. Hi Vjeko!
    Very Good Post!!
    I am quite new to NAV and I am wondering if it make sense to modify preview posting to avoid table locks.
    You think that’s possible doing small modification (not calling LockTables routine when Preview) on CU 80 ?
    Thank you in advance!

    1. Hi ktast,

      You cannot avoid table locks. All data in NAV is written to actual tables, and data writes require locking to take place. Modifying all this to use temporary tables instead would be quite a lot of work, most likely futile. So, no, I don’t think it makes much sense to attempt or do.

      /Vjeko

Leave a Reply