4
\$\begingroup\$

I'm still in the process of learning Java / spring and I think I'm getting better. Now at this point I'm able to build a rest api BUT I'm at a lost at how to ensure I've no concurrency issues . I've read quite a bit of documentation on having my classes stateless and I "think" I've achieved that.

Could I get a code review please to see if anything here would be an issue should there be say xxx concurrent requests to the controller below.

Below i have a POJO called User:

@Getter
@Setter
@Document(collection = "UserProfiles")
public class User {
    @Id
    @JsonIgnore
    private String _id;

    @JsonView({ProfileViews.Intro.class, ProfileViews.Full.class})
    private String userId;

    @JsonView({ProfileViews.Intro.class, ProfileViews.Full.class})
    private String name;

    @JsonView({ProfileViews.Intro.class, ProfileViews.Full.class})
    private String displayName;

    @DBRef
    @JsonView({ProfileViews.Full.class})
    private UserInterests personalInterests;

    @DBRef
    @JsonIgnore
    private ProfileFollows profileFollowDetails;

}

@Getter
@Setter
@Document(collection = "ProfileFollows")
public class ProfileFollows {
    @Id
    //Id of The Mongo Document
    private String id;

    //The Id of the User Profile who owns the document
    private String userId;

    //A list containing the Ids of the Users who have followed the Profile belonging to userId
    private List<String> profileFollowedByUserIds;

    //A list containing the Ids of the Profiles the current user has followed
    private List<String> profileFollowingByUserList;
}

And here is my Service layer where I create and update the user

@Service
public class UserService {

    @Autowired
    UserDal userDal;

    public User createNewUserAccount(String userId, String userName) {
        //check If userId already in DB

        if (checkIfUserIdExits(userId)) {
            throw new UserAlreadyExistsException("Cannot create User with Id { " + userId + " }, a user with this Id already " +
                    "exists");
        }

        //Create a Empty / Base New User Object
        User newUser = new User();
    
        UserInterests userInterests = new UserInterests();
        userInterests.setUserId(userId);
        userInterests.setPersonalInterestsExtras(null);
        userInterests.setCreatedDate(Instant.now());
        userInterests.setLastUpdatedAt(Instant.now());
        userInterestsDAL.save(userInterests);

        newUser.setPersonalInterests(userInterests);

        ProfileFollows userProfileFollows = new ProfileFollows();
        userProfileFollows.setUserId(userId);
        userProfileFollows.setProfileFollowedByUserIds(new ArrayList<>());
        userProfileFollows.setProfileFollowingByUserList(new ArrayList<>());
        newUser.setProfileFollowDetails(profileFollowsDAL.save(userProfileFollows));


        newUser.setUserId(userId);
        newUser.setDisplayName(generateUserDisplayName(userName));
        newUser.setCreatedDate(Instant.now());
        newUser.setLastUpdatedAt(Instant.now());

        //save the new User Profile to the DB
        return userDal.save(newUser);
    }

Here is my UserDAL:

public interface UserDal {
    /**
     * Method to check if a user exists with a given user Id
     * @param Id -- Id of user to look up where id is a string
     * @return
     */
    Boolean existsById(String Id);

    /**
     * Method to save a user to the DB
     * @param user -- User object to save to the DB
     * @return
     */
    User save(User user);
}

My User Repository / DALImpl:

@Repository
public class UserDALImpl implements UserDal {

    private final MongoTemplate mongoTemplate;

    @Autowired
    public UserDALImpl(MongoTemplate mongoTemplate) {
        this.mongoTemplate = mongoTemplate;
    }

  @Override
    public User save(User user) {
        return mongoTemplate.save(user);
    }

And lastly my controller:

@RestController
@RequestMapping("/profile")
public class CreateProfileController {

    @Autowired
    public CreateProfileController() {
    }

    @Autowired
    UserService userService;

    @ApiOperation(value = "Allows for the creation of a user Profile")
    @PostMapping("/create")
    public User createUserProfile(@RequestParam(name = "userId") String userId,
                                  @RequestParam(name = "displayName", required = true, defaultValue = "AnonymousDev") String displayName) {

        if (userId.equals("")) throw new BadRequestException("UserId cannot be blank");

        if (userService.checkIfUserIdExits(userId)) {
            throw new UserAlreadyExistsException("Unable to create user with Id { " + userId + " }, the " +
                    "userId already exists");
        }
        return userService.createNewUserAccount(userId, displayName);
    }
}

Thank you for your time

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Is the code currently working as expected? That is one of the requirements for asking a question on the Code Review Community. Please read [How do I ask a good question?(codereview.stackexchange.com/help/how-to-ask). \$\endgroup\$
    – pacmaninbw
    May 29, 2021 at 18:47
  • \$\begingroup\$ Ah sorry I should have read the guide. But to answer your question it "seems" to be working as expected. However i'm slightly junior here so I'm not sure if there's something that will bite me from a thread safety POV \$\endgroup\$
    – MetaCoder
    May 29, 2021 at 19:13

2 Answers 2

4
\$\begingroup\$

Welcome to Code Review, because Spring demands to the user the responsibility of avoiding concurrency issues better always check if the possibility of concurrent issues exists. About your CreateProfileController controller:

public class CreateProfileController {

    @Autowired
    public CreateProfileController() {
    }

    @Autowired
    UserService userService;
}

The possibility of concurrency issues is related to the field userService but it is directly handled by Spring with the use of the @Autowired annotation so the possibility of accidentally mutating it in someway is excluded (it is a singleton with no setter methods). There's no possibility of storing data between successive requests like in an http session because the controller is also handled by Spring and you haven't defined the session scope of the controller. I haven't checked it but I think you could rewrite your controller bounding the @Autowired annotation to the constructor like below:

public class CreateProfileController {

    @Autowired
    public CreateProfileController(UserService userService) {
       this.userService = userService;
    }

    private UserService userService;
}

About the other classes, I find a little strange you haven't used the @GeneratedValue annotation with the present @Id annotation. Without combining them a generic api user has to manually assign the id in the request like your code below:

@PostMapping("/create")
    public User createUserProfile(@RequestParam(name = "userId") String userId,...)
    

So the user knows informations about his internal database id and for me this is bad, unless you are thinking that userId is like an email address with a one to one relationship with the profile; in this case I think there are should be no issues but I'm not sure about it.

\$\endgroup\$
7
  • \$\begingroup\$ Wow thank you for your answer. This is my first time using stack codereview and I'm amazed by the detailed answer so thank you. \$\endgroup\$
    – MetaCoder
    May 30, 2021 at 15:44
  • \$\begingroup\$ Ok so I think i'm ok and i'll take your advice with the @Autowired. Also you make a very good point in general about the userId in the POST request. This endpoint is only called from Auth0 and has a scope permission on it (sorry i shouldn't have left that out). Once again thank you for taking the time! \$\endgroup\$
    – MetaCoder
    May 30, 2021 at 15:45
  • \$\begingroup\$ Actually one question if i may. In the above code, is one way I could generate a concurrency issue (just for my learning) by declaring a class level variable (imagine Integer count) and having THAT accessed / mutated by methods in the Service class \$\endgroup\$
    – MetaCoder
    May 30, 2021 at 15:47
  • 1
    \$\begingroup\$ @Philban You are welcome and your example of an int count hits the point of a pretty sure concurrency issue, for example if your service read the count value to obtain a new record id an api user would risk to have his request refused because an already present record with the same id in your database. \$\endgroup\$ May 30, 2021 at 15:57
  • \$\begingroup\$ Sorry for the delayed response, I've been off on leave plus I killed my laptop (yay) excellent thanks for your response again \$\endgroup\$
    – MetaCoder
    Jun 24, 2021 at 14:01
1
\$\begingroup\$

A few additional areas to consider, not directly related to you concurrency question...

UserAlreadyExistsException

This takes an argument that appears to be an error message. The message that's being passed in feels very much like it's essentially the name of the exception, along with the userId that already existed. If you own this exception, consider changing it so that the userId is passed into the constructor instead and the exception itself is responsible for constructing the error message (if it's necessary). This would simplify each of the callers...

Duplication

Both your Service and Controller perform validation on the UserId to see if it exists:

if (userService.checkIfUserIdExits(userId)) {
        throw new UserAlreadyExistsException("Unable to create user with Id { " + userId + " }, the " +
                "userId already exists");
}

The error message reported is very slightly different between each of the calls, however I'd consider if you really need to check it in both places. It seems like the Controller could probably trust the service to perform the exist validation if it's required. Think about what it is each of the classes is responsible for. To me, the Controller class is responsible for orchestrating the API, it doesn't actually do any work, it passes requests to the right destination and responses back to the client.

I'd also consider if it actually makes sense for checkIfUserIdExits to return a flag. Do you have any code that calls this method and doesn't throw if the user exists? If not then it might be better for the check method to throw directly.

Typo

checkIfUserIdExits should probably be checkIfUserIdExists...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.