24/03/2017

Report from the battlefield #10 - fuck-up with AutoMapper

Home



Have you ever heard or used AutoMapper? What a question, of course you have. And in the very unlikely scenario that you haven't, it's the object to object mapper that allows you to map probably everything. In short no more manual, boring, tedious, error-prone mapping.

However, the great power comes with great responsibility. In the recent time, I had an occasion to fix 2 difficult to track bugs related to improper usage of AutoMapper. Both issues were related to the feature of AutoMapper which according to me is almost useless and at least should be disabled by default. Let's look at the following 2 classes and testing code:
public class SomeSourceClass
{
   public Guid Id { get; set; }
   public string IdAsString => Id.ToString();
   public string Value { get; set; }
}

public class SomeDestinationClass
{
   public Guid Id { get; set; }
   public string IdAsString => Id.ToString();
   public string Value { get; set; }
}

class Program
{ 
   static void Main()
   {
      Mapper.Initialize(config => config.CreateMap<SomeSourceClass,SomeDestinationClass>>());
      
      var src = new SomeSourceClass { Id = Guid.NewGuid(), Value = "Hello" };
      var dest = Mapper.Map<SomeDestinationClass>(src);

      Console.WriteLine($"Id = {dest.Id}");
      Console.WriteLine($"IdAsString = {dest.IdAsString}");
      Console.WriteLine($"Value = {dest.Value}");
   }
}
This works as a charm. If you run this example, you should see output like that:

Id = a2648b9e-60be-4fcc-9968-12a20448daf4
IdAsString = a2648b9e-60be-4fcc-9968-12a20448daf4
Value = Hello

Now, let's introduce interfaces that will be implemented by SomeSourceClass and SomeDestinationClass:
public interface ISomeSourceInterface
{
   Guid Id { get; set; }
   string IdAsString { get; }
   string Value { get; set; }
}

public interface ISomeDestinationInterface
{
   Guid Id { get; set; }
   string IdAsString { get; }
   string Value { get; set; }
}

public class SomeSourceClass: ISomeSourceInterface { /*... */}

public class SomeDestinationClass : ISomeDestinationInterface { /*... */}
We also want to support mappings from ISomeSourceInterface to ISomeDestinationInterface so we need to configure AutoMapper accordingly. Otherwise the mapper will throw an exception.
Mapper.Initialize(config =>
   {
      config.CreateMap<SomeSourceClass, SomeDestinationClass>();
      config.CreateMap<ISomeSourceInterface, ISomeDestinationInterface>();
   });

var src = new SomeSourceClass { Id = Guid.NewGuid(), Value = "Hello" };
var dest = Mapper.Map<ISomeDestinationInterface>(src);

Console.WriteLine($"Id = {dest.Id}");
Console.WriteLine($"IdAsString = {dest.IdAsString}");
Console.WriteLine($"Value = {dest.Value}");
If you run this code, it'll seemingly work as the charm. However, there is a BIG PROBLEM here. Let's examine more carefully what was written to the console. The result will look as follows:

Id = a2648b9e-60be-4fcc-9968-12a20448daf4
IdAsString =
Value = Hello

Do you see a problem? The readonly property IdAsString is empty. It seems crazy because IdAsString property only returns the value of Id property which is set. How is it possible?

And here we come the feature of AutoMapper which according to be should be disabled by default i.e. automatic proxy generation. When AutoMapper tries to map ISomeSourceInterface to ISomeDestinationInterface it doesn't know which implementation of ISomeDestinationInterface should be used. Well, actually no implementation may even exists, so it generates one. If we check the type of dest property we'll see something like:

Proxy<ConsoleApplication1.ISomeDestinationInterface_ConsoleApplication1_Version=1.0.0.0_Culture=neutral_PublicKeyToken=null>.

Initially this function may look as something extremely useful. But it's the Evil at least because of the following reasons:
  • As in the example, the mapping succeeds but the result object contains wrong data. Then this object may be used to create other objects... This can lead to really difficult to detect bugs.
  • If a destination interface defines some methods, a proxy will be generated, but the mapping will fail due to System.TypeLoadException.
  • It shouldn't be needed in the well written code. However, if you try to cast the result of the mapping to the class, then System.InvalidCastException exception will be thrown.
The ideal solution would be to disable this feature. However, I don't know how :( The workaround is to explicitly tell AutoMapper not to generate proxies. To do that we need to use As method and specify which concrete type should be created instead of a proxy.

The final configuration looks as follows. It's also worth mentioning that in this case we actually don't need to define mapping from SomeSourceClass to SomeDestinationClass. AutoMapper is clever enough to figure out that these classes implements interfaces.
Mapper.Initialize(
   config =>
   {
      config.CreateMap<ISomeSourceInterface, ISomeDestinationInterface>().As<SomeDestinationClass>();
   });


AutoMapper proxy generation feature is the Evil.


*The picture at the beginning of the post comes from own resources and shows Okonomiyaki that we ate in Hiroshima. One of the best food we've ever eaten.

15/03/2017

Report from the battlefield #9 - async/await + MARS

Home



This post from Report from the battlefield series will be about my own mistake. It is related to async/await and MARS i.e. Multiple Active Result Sets. async/await allows us to use asynchronous programming more easily. MARS is a feature of MSSQL that allows us to have more than one pending request opened per connection at the same time. For example, it may be useful if we have 2 nested loops i.e. internal and external. External loops iterate through one result set and the internal one through another. Ok, but you probably wonder what MARS has in common with async/await.

A few days ago my application started failing due to InvalidOperationException exception with the additional message saying that The connection does not support MultipleActiveResultSets. Well, I used MARS in the past so I simply enabled it in the connection string by setting MultipleActiveResultSets attribute to true.

However, later I realized that my application should not require MARS at all so I started digging into what was wrong. It turned out that the problem was related to my silly mistake in using async/await. Let's look at the following simplified version of the problematic code. We have a trivial Main method:
static void Main()
{
   Start().GetAwaiter().GetResult();
}
Start is an async method responsible for opening a connection to DB and executing other async methods:
private static async Task Start()
{
   using (var c = new SqlConnection(ConnectionString))
   {
      c.Open();

      await Func1(c);
      await Func2(c);
      await Func3(c);
   }
}
Func1, Func2 and Func3 are responsible for reading data and processing them. In our case, for simplification, they all will do the same thing:
private static async Task Func1(SqlConnection c) => await ReadData(c);
private static async Task Func2(SqlConnection c) => ReadData(c);
private static async Task Func3(SqlConnection c) => await ReadData(c);
And here is the ReadData method. It's also simple. It simply reads data from a table:
private static async Task ReadData(SqlConnection c)
{
   var cmd = c.CreateCommand();

   cmd.CommandText = "SELECT * FROM dbo.Fun";

   using (var reader = await cmd.ExecuteReaderAsync())
   {
      while (await reader.ReadAsync())
      {
         // Process data
      }
   }
}
If you run this code, the aforementioned InvalidOperationException exception will be thrown in the line with ExecuteReaderAsync. The question is why? Well, in this short code it is rather easy to spot that in Func2 method await is missing before ReadData. But, do you know why it is a problem? If not, don't worry it's a little bit tricky.

Here is an explanation. Without await the simplified flow is as follows:
  • ...
  • Start executes Func2.
  • Func2 executes ReadData.
  • ReadData executes ExecuteReaderAsync.
  • ReadData awaits for the result from ExecuteReaderAsync.
  • The control returns to caller i.e. Func2.
  • However, Func2 does not use await so it returns completed task to Start method.
  • From the point of view of Start processing of Func2 is finished so it executes Func3.
  • Func3 executes ReadData
  • The previous call to ReadData may be still in progress.
  • It also means that ReadData will call ExecuteReaderAsync when another result set is still being processed.
  • The exception is thrown.
Adding missing await fix the problem. Thanks to that the task returned from Func2 will not be completed until call to ReadData is over. And if so Start will not execute Func3 immediately. The final well known conclusion is:

Always async/await all the way down.


*The picture at the beginning of the post comes from own resources and shows Laurel forest on La Gomera.

08/03/2017

Roslyn and unit tests suck

Home


Title: Imperial Gardens in Tokyo, Source: own resources, Authors: Agnieszka and Michał Komorowscy

I'm working on the project where I have an opportunity to use Roslyn compiler as a service. It is very good :) However yesterday it took me more than 2 hours to write working unit tests (based on MSTest) for my code! Here are some tips that may save your time.

Let's start with the simle thing. When I run unit tests for the first time the following exception was thrown:

System.IO.FileNotFoundException: Could not load file or assembly 'System.Runtime...' or one of its dependencies.

To fix this problem I simply installed the following packages via Nuget:
  • Microsoft.CodeAnalysis.CSharp
  • Microsoft.CodeAnalysis.CSharp.Workspaces
Later it was harder. In my code I use MSBuildWorkspace.OpenSolutionAsync and MSBuildWorkspace.OpenProjectAsync methods to respectively open the entire solution or a single project for further processing.

The next issue was that the first method called from within a unit test was returning an empty solution i.e. without any projects. Whereas the second one was throwing an exception with the message: The language 'C#' is not supported. What was strange these problems occurred only in unit tests! To investigate a problem I opened Exception settings window in Visual Studio and selected a check box next to Common Language Runtime Exceptions. Then, I run the unit tests one more time and Visual Studio quickly reported the exception in the line with MSBuildWorkspace.OpenProjectAsync:

System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.CodeAnalysis.CSharp.Workspaces...' or one of its dependencies.

It was even more strange because my unit test project was actually referencing Microsoft.CodeAnalysis.CSharp.Workspaces.dll! To double check, I went to the unit tests working directory. It is a folder called TestResults which by default is located in the solution directory. To my surprised this dll was missing!

Fortunately, I reminded myself the similar situation from the past. The problem is that MSTest doesn't copy all assemblies to the output directory by default. As far as I know it tries to figure out which assemblies are really needed by the code being tested. Here, I'm not sure but Microsoft.CodeAnalysis.CSharp.Workspaces.dll may be cumbersome because it is not directly referenced by other Roslyn assemblies. Instead, it is probably loaded dynamically when needed.

To fix a problem you can use the simple hack i.e. use directly any code from Microsoft.CodeAnalysis.CSharp.Workspaces.dll in your unit tests in the following way:

[ClassInitialize]
public static void ClassInitialize(TestContext ctx)
{
   var t = typeof(Microsoft.CodeAnalysis.CSharp.Formatting.LabelPositionOptions);
}

Why did I use LabelPositionOptions? Because majority of types defined in aforementioned assembly is internal and this one was the first public type I found :)

27/02/2017

Report from the battlefield #8 - always remember about the context

Home


Title: Sunrise seen from the top of Mount Fuji, , Source: own resources, Authors: Agnieszka and Michał Komorowscy

I decided to change a little bit a character of Report from the battlefield series. Initially, in this series, I was describing my observations from my work as a reviewer for a recruitment company. Now, I'll be also writing about my findings from my day-to-day work. To start, I'll give you a tip how to log useful information.

I worked with an application that is responsible for monitoring folders. If it detects any new files, they are processed and copied somewhere else. The application logs information like the number of files to be processes, the file that is currently being processed etc. This information are logged with severity Information or Debug. It happens that a given file cannot be copied for example because the file with the same name already exists in the destination directory. In that case .NET throws System.IO.IOException. This exception is caught and logged with the severity Error. The simplified version of the log could look in this way:

INFO - 10 files have been found
INFO - Processing file started
...
INFO - Processing file ended
INFO - Processing file started
ERROR - An error occurred while processing a file: Cannot create a file when that file already exists.
...
INFO - Processing file ended
...

It's good that many important information are logged. However, there is a major issue with this log. The lack of the context! For example we know that some files have been processed but we don't know which exactly. This log should look as follows (I used red color to mark changes):

INFO - 10 files have been found in the directory 'C:\Input'
INFO - Processing file 'C:\Input\a.txt' started
...
INFO - Processing file 'C:\Input\a.txt' ended
INFO - Processing file 'C:\Input\b.txt' started
ERROR - An error occurred while processing a file: Cannot create a file when that file already exists.
...
INFO - Processing file 'C:\Input\b.txt' ended
...

It looks much better. Based on the log we can figure out which directory was monitored, which files have been processed successfully and which not. However, it's not everything. There is one more subtle problem here. What if messages with the severity Information or lower won't be logged (for example because of performance issues) and an error will be reported. In this case we'll get the following log:

...
ERROR - An error occurred while processing a file: Cannot create a file when that file already exists.
...

It's better than nothing but again we don't know processing of which file has actually failed. The expected result is:

...
ERROR - An error occurred while processing a file 'C:\Input\b.txt': Cannot create a file when that file already exists.
...

To sum up, always remember about the context when logging.

20/02/2017

Interview Questions by MK #8

Home


Title: Sunset seen from the top of Mount Fuji, Source: own resources, Authors: Agnieszka and Michał Komorowscy

This is the first post from Interview Questions by MK series for a long time. The motivation to write it was a short talk with my colleague. His company really want to hire new .NET developers. The situation on the market is difficult for employers so they are also considering juniors without experience. And still they have a problem to find someone. Why?

The requirements are not extremely high. I'd say that they are standard. They don't demand God knows what. The ideal candidate doesn't have to: know all formatting options available in .NET, enumerate classes in System.DirectoryServices namespace, tell about all new features introduced in any .NET version or any other thing that can be checked in the documentation within seconds. However, they want someone with general knowledge. What I mean by that?
  • It's good to know how to write a class, properties, derived a class... but it's also good to understand and can explain principals of the object oriented programming. For example could you tell why OOP is better than the procedural programming? Or maybe it isn't? Could you justify why encapsulation is actually a good thing?
  • You don't have to know all possible collections available in .NET API but it's worth knowing some of them and their characteristics. Just to mention the list, the dictionary, the stack or the queue.
  • You don't have to be very good in algorithms but knowing how to search a binary tree is not the rocket science.
  • Writing a code that compiles without errors is only a first step. You should also know how to write a readable and a maintainable code. This knowledge comes from experience but at the beginning you should hear about refactoring, knows that the method with 50 parameters is not the best choice...
  • It's not a problem if you have never worked with the continuous integration but you should at least know that there is something like that.
  • As a developer you'll probably not work directly with IT infrastructure but knowing what is the load balancing or the computer cluster does not seem very demanding.
  • ...
I can continue and continue this enumeration. According to me these are basic things, but still many candidates don't know them. Sometimes even developers with a few or more years of experience.

If you are one of them and you want to have better chances on the job market, I recommend one simple thing i.e. reading books, blogs, web sites... whatever you want. Several minutes (better more) every day, regularly, will make a big difference.

You may also say that you don't care because you'll get a job anyway. Well, it's true at least for now. Nonetheless it'll be only some job.