Code review red flag: Where is the missing code?
I recently had what amounted to a drive by code review. I was looking into code that wasn’t committed or PR. Code that might not have been even saved to disk at the time that I saw it. I saw that while working with the developer on something completely different. And yet even a glace was enough to cause me to pause and make sure that this code will be significantly changed before it ever move forward. The code in question is here:
What is bad about this code? No, it isn’t the missing ConfigureAwait(false), in that scenario we don’t need it. The problem is in the very first line of code.
This is meant to be public API. It will have consumers from outside our team. That means that the very first thing that we need to ensure is that we don’t expose our own domain model to the outside world.
There are multiple reasons for this. To start with, versioning is a concern. Sure, we have the /v1/ in the route, but there is nothing here that would make break if we changed our domain model in a way that a third party client relies on. We have a compiler, we really want to be able to use it.
The second issue, which I consider more important, is that this leaks information that I may not really want to share. By exposing my full domain model to the outside world, I risk quite a bit. For example, I may have internal notes on the support ticket which I don’t want to expose to the public. Any field that I expose to the outside world is a compatibility concern, but any field that I add is a problem as well. This is especially true if I assume that those fields are private.
The fix is something like this:
Note that I have class that explicitly define the shape that I’m giving to the outside world. I also manually map between the internal and external fields. Doing something like auto mapper is not something that I want, because I want all of those decisions to be made explicitly. In particular, I want to be sure that every single field that I share with the outside world is done in such a way that it is visible during PR reviews.
Comments
I am a big fan of DTO for api for this reason (and overfetching), but don't you think it should be handled at an higher level ? Like some attributes that changes serialization depending on the user or the current query ?
Remi,
Attributes that change serializer settings sounds like a recipe for violating single responsibility principle and a great way to add a lot of complexity.
There seems like a security issue as well with the org == check. Not entirely sure what the purpose of that is, but those sort of checks belong in a domain service to wrap any security around it, and to make sure people are accessing only what they are supposed to.
Just wondering if a take parameter should also be added to the method's signature in order to give proper paging support. Or is the session.Query already limiting in some way the maximum amount of retrieved data?
Can you give me an example where the usage of automapper makes sense?
Wouldn't returning DTO be better for unit testing. It is difficult to test method returning IActionResult.
Stephen,
I strongly oppose creating services just for this purpose. There are logic and security issues here that you need to deal with, but you can also do verification of such in a cross cutting manner. Modifying the architecture and making it a lot more rigid isn't a good idea.
Zoltan,
In this case, I didn't bother with it, but yes, that would be generally advisable. The amount of tickets for each user is probably < 10 in most cases.
Andres,
Probably on the other side? When you read from DTO to you model, but then again, I would like it explicit. In general, if you find yourself mapping between types so often you need a tool for that, there is something wrong here.
Dalibor,
It is easy enough to cast the IActionResult to the right type and get the resulting value.
Is it in this case? It is an anonymous return type, how would you cast it?
Dalibor,
I don't follow? It is * PublicTicketDto*, that is a known type
Oops, My error, I was blind and did not see the type. Please ignore my last comment.
Just out of curiosity why not simply have the method return a
Task<List<PublicTicketDto>>
instead ofTask<IActionResult>
? The way I see it the only benefit of returning IActionResult is if you want to manipulate the HTTP return code and I usually do not need this on API methods (throwing an exception works fine for me). Thanks.Dalibor,
Yes, that is possible, certainly.
I hope in a real code review you'd have a decent conversation about the trade off between increasing the maintenance burden (which you've done) versus exposing a domain model. That's much more helpful than imposing a blanket rule (which can lead to blind overengineering).
Code reviews don’t exist in vacuum, at least not in Hibernating Rhinos. They are part of an ongoing process of making our code better. We try to create a culture in which code reviews are a two way street. A code review comment can be either accepted, clarified or used to start a discussion on a particular concern.
I found that having a bland list of rules saying: “Thou shall do so” is a good way to get to exactly what you said you wanted, but not what you actually wanted.
In this particular case, we are dealing with an API that is going to be open to the public and as such the set of concerns that we have to deal with are quite different from our usual behavior.
Comment preview