restapi: Fix NullPointerException when using follow parameters

[Artemi10]

oVirt RESTful API may produce NullPointerException while processing requests that include follow parameters. This leads to Internal Server Error (500 status code) is returned to the client.

Two specific scenarios were identified that can trigger this issue:

1. Non-existent followed entities: The requested entity does not have the specified followed entity.

• Before fix:
  
• After fix:
  

2. Null followed entities: The followed entity exists in the model but is set to null.

• Before fix:
  
• After fix:
  This fix adds appropriate null checks to ensure that followed entity exists in requested entity, and it is not null.
  

Changes introduced with this PR

• Added check to ensure that getter method for the followed entity is not null

• Added check to ensure that fetched followed entity is not null

• Added tests covering both scenarios

Are you the owner of the code you are sending in, or do you have permission of the owner?

Yes

[JasperB-TeamBlue]

Nice work, and sorry for the long time until there was any response on this matter.

The issue

The end user creates a bad request to which the engine can't respond. It returns a server error because internally there is a NullPointerException.

Your solution

Ignore the bad request formed by the user and return the requested information without the follows.

What should be done

It should be best practice that you don't ignore the exception but rather capture it and report it as a WebFaultException with response status BAD_REQUEST.
This would capture the internal server error and return more info to the end user about what is wrong about what they are requesting, not hide it and return the value regardless.

I am open to discussion about this, but hiding the issue does not feel like the appropriate response.

[0ffer]

The issue

The end user creates a bad request to which the engine can't respond. It returns a server error because internally there is a NullPointerException.

This not fully correct. Main issue to solve here is to able to response for entities without followed objects (described case 2)
Without these improvements, it is not possible to get entities with and without related objects using the same query.
The first case is the results obtained along the way. We think that the resulting problem is less significant than not being able to return entity at all.

But I agree with you. It is better to return Bad Request when there no linked entity with given name (case 1).
We will look how to implement such logic.

[Artemi10]

What should be done

It should be best practice that you don't ignore the exception but rather capture it and report it as a WebFaultException with response status BAD_REQUEST. This would capture the internal server error and return more info to the end user about what is wrong about what they are requesting, not hide it and return the value regardless.

Fix the first scenario where “The requested entity does not have the specified followed entity” so that it returns an exception with HTTP 400 (Bad Request).

• For collection entities

• For single entities

[JasperB-TeamBlue]

How you are using this WebFaultException is incorrect.
Status.BAD_REQUEST should be Response.Status.BAD_REQUEST. also it should be best practice that the fault string that is thrown is localized by using the function localize(). Great start already but you should look into how it is used in other parts of the codebase.

[JasperB-TeamBlue]

The WebFaultException should be improved to be localized and just general build of the exception. Otherwise agree on the way the problem is now being tackled.

[Artemi10]

Thanks for the feedback. I’ve updated the WebFaultException to use a localized message (INCORRECT_FOLLOW_LINK), following the existing pattern used in other parts of the codebase. The overall approach remains unchanged.

• For collection entities

• For single entities

[JasperB-TeamBlue]

Could you please rebase your 3 commits into one? And also update the commit msg to include information about the usage of the BAD_REQUEST return.

[Artemi10]

Thanks for the feedback.
I’ve squashed 3 commits into a single one and updated the commit message to explicitly describe the usage of the BAD_REQUEST (400) response for invalid follow parameters.

[JasperB-TeamBlue]

The previous test in this suite did indeed declare throw SecurityException, IllegalArgumentException but since these two exceptions are unchecked because they derive from RunTimeException. Best practice is to not declare them.

[Artemi10]

Agreed. Removed unnecessary throws declarations for unchecked exceptions from the tests introduced in this PR.