3

I wrote a controller that must return an unique String. The requirement is that two calling of this controller never return the same String, even after years and even if the code will scale to more VMs.

My question is if the following code is correct to achieve to declared purpose, or if you have any hint.

Controller:

@RestController
public class UtilityController {

    @Autowired
    UtilityServices utilityServices;

    @GetMapping("/uniqueIdentifier")
    @ResponseBody
    public String uniqueIdentifier() {
        return utilityServices.getUniqueIdentifier();
    }

Service:

@Service
public class UtilityServices {

    @Autowired
    private UniqueIdRepository uniqueIdRepository;

    @Transactional
    public String getUniqueIdentifier() {
       String uniqueId = RandomString.getSecureRandomString();
       while (uniqueIdRepository.existsById(uniqueId)) {
           uniqueId = RandomString.getSecureRandomString();
       }
       uniqueIdRepository.save(new UniqueId(uniqueId));
       return uniqueId;
    }
}

Entity:

@Entity
@AllArgsConstructor
@NoArgsConstructor
@Getter
@Setter
@EqualsAndHashCode
@ToString
public class UniqueId implements Serializable {

    @Id
    private String uniqueId;   

}

Repository:

public interface UniqueIdRepository extends CrudRepository<UniqueId, String> {

}

That's all. I omit the code the RandomString class because it's not relevant in this context: I wrote a code based on SecureRandom, it is very likely that each time it returns a different String, but I have no guarantees about it. Let's assume that sooner or later my RandomString.getSecureRandomString() method can return the same String.

I'm not sure if the @Transactional annotation guarantees that the getUniqueIdentifier() method never throws an error.

Francesco Galgani
  • 6,137
  • 3
  • 20
  • 23
  • I don't know if it's relevant, anyway the database is MySql. – Francesco Galgani May 23 '20 at 09:09
  • Don't, as this code is flawed as there is no garantuee it is really unique. Just use a UUID or something like twitters snowflake algoritme. Don't try to reinvent the wheel. – M. Deinum May 23 '20 at 09:16
  • Okay, but please help me understand. Relying on a different algorithm without understanding what's wrong with mine is not educational for me. My controller, even if it reinvents the wheel, will it ever return the same string? Does the use of the database and the @Transactional annotation, in this case, give me full guarantees? – Francesco Galgani May 23 '20 at 09:27
  • 1
    No. It doesn't. The transactional doesn't prevent multiple transactions on the same table. One could result in an error. If 2 tx. at the same time generate the same id, they both don't see it, try to persist and one will throw an exception. But there is already a perfect solution by using a UUID no need to query, just generate and store (you could even generate that in the DB if you want). – M. Deinum May 23 '20 at 09:30

1 Answers1

1

The much better idea at your case will be using UUID:

Thus, anyone can create a UUID and use it to identify something with near certainty that the identifier does not duplicate one that has already been, or will be, created to identify something else. Information labelled with UUIDs by independent parties can, therefore, be later combined into a single database or transmitted on the same channel, with a negligible probability of duplication.

@Service
public class UtilityServices {
    @Autowired
    private UniqueIdRepository uniqueIdRepository;

    @Transactional
    public String getUniqueIdentifier() {
       String uniqueId = String.format("%s-%s",
            RandomStringUtils.randomAlphanumeric(4),
            UUID.randomUUID().toString().replace("-", "")
       );
       // you could left this check 
       while (uniqueIdRepository.existsById(uniqueId)) {
           uniqueId = UUID.randomUUID().toString().replace("-", "");
       }
       uniqueIdRepository.save(new UniqueId(uniqueId));
       return uniqueId;
    }
}

BTW you could use @Data for Model:

@Data
@Entity
@NoArgsConstructor
@AllArgsConstructor
public class UniqueId implements Serializable {
    private static final long serialVersionUID = 0L;
    @Id
    private String uniqueId;   
}

And don't forget about serialVersionUID.

Useful references:

catch23
  • 17,519
  • 42
  • 144
  • 217
  • Thank you. Specifically, how would you correct my code? – Francesco Galgani May 23 '20 at 09:14
  • `@Data` is not a good choice for an entity (mainly due to the generated `equals`, `hashCode` and `toString` which could lead to eager init issues (for one). A UUID is already unique, so no need to query. Just generate and store (or let it be generated by the DB). – M. Deinum May 23 '20 at 09:36
  • I'd like to say something against the use of UUID: according to their specification, UUIDs are not designed to be unpredictable, and should not be used as session identifiers (source: https://stackoverflow.com/a/41156/1277576). I also have doubts about the use of date-time details when multiple VMs are working simultaneously. – Francesco Galgani May 23 '20 at 09:56
  • 1
    @FrancescoGalgani you are partly right. However, `The total number of unique UUID keys is 2128 = 25616 or about 3.4 × 1038. This means that generating 1 trillion keys every nanosecond, you can go through all possible values ​​in just 10 billion years` from Wiki on another language. Or from en `the probability to find a duplicate within 103 trillion version-4 UUIDs is one in a billion`. I agree for date-time generation for parallel VM, if you have such a case. – catch23 May 23 '20 at 10:18
  • 1
    I have a couple of remarks about my code, after all the previous comments: @Transactional is useless, because `getUniqueIdentifier()` will never launch an exception. This is because `uniqueIdRepository.save()` is designed to perform insert/update functions, and not just insert. So saving two entities with the same uniqueId doesn't throw any error. In this sense, even using the database to guarantee the uniqueness of the String is useless, at least the way I thought. – Francesco Galgani May 23 '20 at 10:24
  • 1
    @FrancescoGalgani updated answer by adding a prefix to generated UUID. To be more sure that it will be really unique. – catch23 May 23 '20 at 10:26