Horreum icon indicating copy to clipboard operation
Horreum copied to clipboard

Method `TestService.getByNameOrId` might return wrong result

Open lampajr opened this issue 9 months ago • 2 comments

Describe the bug

The endpoint /api/test/byName/{name} might return wrong result if there exist a Test having as name the id of another test (which seems possible).

Taking a look at the implemention:

   public Test getByNameOrId(String input){
      TestDAO test = null;
      if (input.matches("-?\\d+")) {
         int id = Integer.parseInt(input);
         // there could be some issue if name is numeric and corresponds to another test id
         test =  TestDAO.find("name = ?1 or id = ?2", input, id).firstResult();
      }
      if (test == null) {
         test =  TestDAO.find("name", input).firstResult();
      }
      if (test == null) {
         throw ServiceException.notFound("No test with name or id " + input);
      }
      return TestMapper.from(test);
   }

I see a couple of issues:

  1. If name matches the id of another test, the query will give two results and the code will simply get the first one, which might be wrong!
  2. I think the second if stmt is useless as the query that is executed is exactly the left side of the previous or statement, so if it was null before it will be also here.

To Reproduce

  1. Create a test and save the id
  2. Create another test having as name the previous test's id
  3. Get by name or id: curl -X GET 'http://localhost:8080/api/test/byName/{name}'
  4. You should see that you might get wrong result

Version

What is the version of Horreum ?

0.12.2 and master

lampajr avatar May 09 '24 12:05 lampajr

I think we should give a precedence to either name or id - maybe in the case the provided input string is full numeric let's give id precedence and if not found, fallback to the name.

Alternatively we could restrict Test name to be alphanumeric, this way we remove the possibility to have collision between ids and names.

lampajr avatar May 09 '24 13:05 lampajr

Worth noting that this endpoint is not currently called by the UI, therefore I think this could be considered as low priority.

lampajr avatar May 22 '24 08:05 lampajr