Handle warnings

[andreaskrath]

Description

Currently, there are a lot of warnings in the code base, and it would make sense to clean some of this up, as it makes it almost impossible to get anything valuable from a build log.

Generally speaking there are a couple of different categories of warnings, and due to the sheer amount of warnings it might make sense to remove these warnings in smaller batches.

For the most part, the warnings that the C# compiler emits should be considered errors, as most warnings are an indication of a possible exception during runtime.

Redundant Conditional Checks

Conditional checks with no purpose, for example, most of the Controllers perform data validation checks before processing the request itself. However, a lot of these checks are redundant as they check whether a given variable is null, when the variable is not nullable. This essentially results in tens, if not hundreds of redundant conditional checks, as these values are never allowed to be null in the first place, and they end up cluttering the code base needlessly.

I am unsure if Visual Studio highlights these conditional checks as redundant, but Rider does, so using an IDE that highlights these warnings would be a good starting point towards a resolution.

Note: this does not only concern the scenarios where if (x == null) is false, there are also multiple instances of the negated scenario, i.e. if (x != null) always being true; these should also be resolved.

This issue also pertains to more than just the controllers, these types of warnings are present throughout the code base.

Nullable Warnings

There are multiple instances of nullable warnings, and these come in many different variants, with to many to cover in this issue description. However, the essence is that type annotation for a given variable, does not match what it might be assigned. In the picture below, the returned GirafUser is not nullable, however there are attempts to return a null from the function. This might generate a NullPointerException, which if handled improperly might lead to a crash, or make it very difficult to understand the flow of the program.

How to resolve these warnings should be considered on a case-by-case basis. The easiest fix would likely be to make every variable nullable, however this is not necessarily a good fix, as it does not always make sense for a variable to be nullable, and nullable variable generate an entirely new set of possible program states to consider.

---

There is also a second variant of these types of warnings, which is primarily seen in the Repositories project when interacting with the database. This warning relates to how objects are retrieved from the database, where the FirstOrDefault method is used most of the time, however the default in these circumstances would be null, and as seen in the example below the return type of the function is not nullable; this again is an example where a NullPointerException might be thrown.

While it is unlikely for most of these queries to return the default, it is still extremely bad practice to assume that only the happy path is possible.

Async Overhead

There are multiple instances of poorly executed async function, an example is shown below. The primary concern here is that code designed to run asynchronously, is running synchronously; this is especially a problem as the web-api is a server, and handling requests and database interactions synchronously is a major concern.

Technically speaking, this does not only concern dealing with warnings. There are also a lot of database interactions that happen synchronously, as seen below, meaning they block the working thread until the interaction is completed, which will halt the entire system. These should be altered to run asynchronously.

The warning in the example above is not relevant to the problem at hand, it is another instance of a nullable warning. Instead, the focus is on, in this case a rather large database interaction happening synchronously. Although it may be obvious, it should be noted that modifying functions like these to be asynchronous will likely break some parts of the code base, and therefore this specific sub-task is probably rather big.

Possible Suggested Solution
Depending on the specific type of warning, a different approach should be used - as such the above explanation should suffice to get the work started.

Sub Tasks

Needless to say, it is extremely important that these changes are idempotent, and should not result in modified observable behaviour. The best way to ensure this, is to use swagger and or an emulator to check all endpoints before and after changes are made; and of course, all test cases should parse.

If all warnings in the code base are resolved, then it might make sense to add the compiler option to interpret warnings as errors, to prevent future developers from introducing these errors again. Keep in mind that this recommendation comes from the point of view, that most warnings the compiler emits are because an exception might be thrown.

The obvious deviation from this, are warnings regarding naming.