Is it a code smell to modify a data transfer object (DTO) within a loop in a Spring service controller?

Question

I have a Spring service that acts as an adapter for another service in my company. The service receives a request to generate push notifications for a given user and needs to call the other service for each device token associated with the user. The request payload to my service and the payload required by the other service are almost identical, with the only difference being the presence of a token field in the latter. The payload of the request is

{
 "fiscal_code": "<unique_id_for_the_person>",
 "send_type": "<the_mobile_app>",
 "token": "<the_token>", // this is present only in the second request
 "note": "",
 "content": {
 "title": "<title_displayed_in_the_push_notification",
 "additional_data": {
 "k1": "v1",
 "k2": "v2"
 }
 }
}

For both the request, I've created a Java class NotificationRequest to represent the request payload, and here is a simplified version of it::

public class NotificationRequest {
 String fiscalCode;
 String sendType;
 String note;
 String token;
 NotificaRequestContent content;
 
 public static class NotificationRequestContent {
 String title;
 Map<String, String> additionalData;
 }
}

In the Spring service controller, I receive a NotificationRequest instance as a payload and use it within a loop to set the device token, for each token before making the call to the other service. Here's a snippet of the controller code:

 @PostMapping("/notification")
 @ResponseBody
 public ResponseEntity<List<NotificationResponse>> sendNotification(@RequestBody NotificationRequest body) {
 List<String> tokens = repository.getFirebaseToken(body.getFiscalCode());
 List<NotificationResponse> notificationResponses = new ArrayList<>();
 tokens.forEach(token -> {
 body.setToken(token);
 NotificationResponse response = notificationService.sendNotification(body);
 notificationResponses.add(response);
 });
 return new ResponseEntity<List<NotificationResponse>>(notificationResponses, HttpStatus.OK);
 }

My concern is whether modifying the same instance of NotificationRequest object within the loop is a code smell. I've read that DTOs should be treated as immutable objects, and I want to ensure that my code follows best practices. If this approach is problematic, what would be a better approach? Should I create a new instance of NotificationRequest for each token, starting from the instance received?

• Greg Burghardt: Where did you read that DTOs should be treated as immutable objects? I haven't read that. Not that I'm the authority on DTOs, of course, I'm just a little surprised by the premise of your question.
• VariabileAleatoria: @GregBurghardt you are right, I probably only read it from random people on the internet. But one thing that raised my doubt is the existence of record since java14
• Greg Burghardt: Are you using records? If not, then what's wrong with what you are doing?
• maple_shaft: This is a security issue as well. Its a lot easier for me to locate the memory address in the jvm for a locally scoped variable. Finding the jvm stack pointer is easy. Creating new instances for each DTO to be sent would create a new section of heap memory for each instance, which would require me to predict that somehow. I would be surprised if this code wasnt flagged by a simple FindBugs report.
• Laiv: DTOs by nature are mutable. DTOs are neither events nor commands no matter how many people strive to believe the opposite.

Answer 1

An object being immutable is a valued quality for very specific reasons. Don't value it without knowing those reasons.

An immutable object can be trusted not to change. That means it's ok to share copies of its reference around, even to other threads, because you know all anything is going to do is read. No writing. This simplifies analyzing what happens to an object throughout its lifetime.

However, your DTO isn't immutable. You can make changes to it. You're simply reluctant to make changes. When you're reluctant for a good reason that has a different name, "No touchy". No touchy objects are mutable objects that can wreck havoc if touched. Since not touching them isn't enforced you have to police how they are used. It's a pain. It's brittle. It's a smell.

But you haven't demonstrated that you have a good reason to be reluctant to touch it yet. To know that you have to go check out everywhere it's used. Any code that knows this thing must be looked at to see if it cares about you touching it. If you write, while some other thread reads, be aware that it can read while you're halfway done writing. Well, unless you write atomically. Update two fields and you can get caught between them. The nasty thing about this is it's amazingly difficult to test for and catch the problem. But I don't know if anything else but this loop knows about this DTO let alone if any threads are involved.

Another concern is when you loop over a collection it's bad form to modify the collection. It can confound your loop. But you seem to just be modifying body not the elements place in the collection. Meh.

If none of that stuff is happening, and it’s easy to tell that it’s not, then being immutable wouldn’t give you much. Touching it should be fine.

But I'll say this. Even if all of that other stuff isn't happening your method name is: sendNotification(). This name gives no hint that it's changing the DTO you passed it. You didn't even warn me in a comment. Don't leave surprises like that buried down in the code.

• maple_shaft: Great answer! I was also going to suggest that this can become a security issue as well. I could attach gdb to the jvm, find the stack pointer, play around with values and capture memory data on the local scoped lambda variable for this object. Now I can manipulate these NotificationResult objects to use a token or message of my choice. Massively insecure.
• candied_orange: @maple_shaft thank you very much. I’d love to add your security concern but I want to be sure I understand it. I thought reflection could crack open an immutable object. So you’re only keeping out honest people. Am I wrong?
• VariabileAleatoria: Thanks for your answer. The service that receives the modified DTO is basically just an http request to an endpoint. No threads are involved. I guess that if I wanted to parallelize the calls, then I would need a request instance for each of them. In that case what would be the way of creating a copy (I guess a new instance of the Map inside content should be generated, otherwise it would be just another reference to the same object)? P.S. I'm not modifying the elements of the collection I'm iterating, I'm just using their value.
• candied_orange: @VariabileAleatoria After reading your code closely it looks like body.setToken(token); is the mutating code. So the concern here isn't the list of String tokens. It's that calling sendNotification(body) changes body which is a nasty surprise. But to know if that's an actual problem you'll need to analyze how body is used elsewhere. The rest of my answer is just mentioning the problems you can avoid by making things immutable. I'm sure they don't all apply here.

Answer 2

While "DTOs must be immutable" is a good rule of thumb, rules are made to be broken. Some places I found it reasonable to do so (I'm retired) were:

• Some database frameworks allow the database to set an auto-incrementing ID as primary key on insert. The key is not known at DTO construction time. The framework may require setters to facilitate this, or perhaps will harness Java reflection to make the DTO mutable.
• Many programmers insist that you create business objects that mirror your DTOs but I have never subscribed to that dogma, as almost always I worked on projects where we had ownership of the database, and the extra layer simply did not earn its keep. This sometimes led to DTOs that had fields that were to do with cross-cutting concerns like caching or values that logically belonged to the entity but required outside data to calculate (For instance a customer record that needs the AccountOnHold field updated from a web service that accessed an external accounting system).

So don't worry about code smell. Beauty is in the nose of the beholder.

Answer 3

I'm a little hesitant to say that DTOs should be immutable. The reason is that I've long been a skeptic of the DTO approach (and its predecessor: the Value Object.) The whole concept that you send an object to IO is misleading. Objects are never transmitted; only wire formats are sent. What a DTO really represents is a simple structure that can easily serialized and deserialized. That's perfectly fine and useful but once you start using them directly as business (domain) objects, it creates a mess because DTOs are degenerate objects.

With that said, what are the benefits of immutability for a DTO? Aside from the undeniable benefits around robustness (especially w/ regard to threading) there is an additional benefit for DTOs: this can greatly simplify the task of determining what parts of a DTO have changed. That is, if you have the original DTO that was retrieved and a new version of the DTO, it's trivial to determine not only whether it has changed but also what specifically changed and how. You can solve this by using listeners to identify and track changes but, in my experience, such solutions are more complex and harder to debug.

If this argument makes sense to you, what does that mean in your specific scenario? I argue not much. Instead of modifying an object directly, you create 'copy on write' methods. For example, you could do something like this:

public class NotificationRequest {
 final String fiscalCode;
 final String sendType;
 final String note;
 public NotificationRequest(String fiscalCode, String sendType, String note) {
 this.fiscalCode = fiscalCode;
 this.sendType = sendType;
 this.note = note;
 }
 public TokenNotificationRequest addToken(String token) {
 return new TokenNotificationRequest(this.fiscalCode, this.sendType, this.note, token);
 }
}
public class TokenNotificationRequest extends NotificationRequest {
 final String token;
 public NotificationRequest(String fiscalCode, String sendType, String note, String token) {
 super(fiscalCode, sendType, note);
 this.token = token;
 }
}

NOTE: I removed in the inner class to keep the example simple.

Whether you want to use inheritance here isn't terribly important. You could also just use the same class for both and make the (final) token null when it isn't needed.

Again, I don't want to claim this is the right way to do DTOs but if you value immutability, copy-on-write 'mutators' are a simple way to keep the code sane.

Answer 4

Yes, I would say it's a smell. There's no reason that what you're doing wouldn't work, but it's not very clean and resistent to change.

And the problem is that you've blurred the lines between what is a @Service and what is a @Controller. For instance, your notificationService isn't a @Controller, so it wouldn't normally be accepting a serializable DTO type in methods of its interface at all. (You are using an interface, right?)

The most correct thing (to allow you to change the downstream without affecting your clients) would be to pass all of the deserialized data as individual arguments to the service:

 @PostMapping("/notification")
 @ResponseBody
 public ResponseEntity<List<NotificationResponse>> sendNotification(@RequestBody NotificationRequest body) {
 List<String> tokens = repository.getFirebaseToken(body.getFiscalCode());
 List<NotificationResponse> notificationResponses = new ArrayList<>();
 tokens.forEach(token -> { 
 NotificationResponse response = notificationService.sendNotification(body.fiscalCode,
 body.sendType, body.note, body.content, token);
 notificationResponses.add(response);
 });
 return new ResponseEntity<List<NotificationResponse>>(notificationResponses, HttpStatus.OK);
 }

However, I understand that conceptually you don't mind if this service is tightly coupled to the downstream implementation - that's just the nature of your webservice- so you can just pass the original body through, but you should still keep the token in a separate argument:

 @PostMapping("/notification")
 @ResponseBody
 public ResponseEntity<List<NotificationResponse>> sendNotification(@RequestBody NotificationRequest body) {
 List<String> tokens = repository.getFirebaseToken(body.getFiscalCode());
 List<NotificationResponse> notificationResponses = new ArrayList<>();
 tokens.forEach(token -> { 
 NotificationResponse response = notificationService.sendNotification(body, token);
 notificationResponses.add(response);
 });
 return new ResponseEntity<List<NotificationResponse>>(notificationResponses, HttpStatus.OK);
 }

And all that is because personally I would still want two separate DTO classes - one for the API you are exposing, and the one for the API you are connecting to. For now, they happen to be very similar. But it should be possible for them to change independently.

In the case where you pass the incoming DTO type to your @Service, it just needs to be able to read the incoming type, and generate the outgoing type. It could just have a simple function to copy all the fields in the initial implementation.

Style note: usually prefer the for (String token: tokens) syntax over forEach with a lambda. forEach is mainly intended for use with method references. Alternatively, if you really want to use the functional style for this section, use a Stream and Collector.