VUU70: Add resource for application layouts #78
Conversation
…tion - Also amend DTOs to differentiate request/response data
# Conflicts: # layout-server/src/main/java/org/finos/vuu/layoutserver/controller/LayoutController.java # layout-server/src/main/java/org/finos/vuu/layoutserver/model/Layout.java # layout-server/src/main/java/org/finos/vuu/layoutserver/model/Metadata.java
- Introduce DefaultApplicationLayoutLoader to lazily load JSON from a static file - Refactor service to use loader - Add integration test case for failure to load default - Rename static JSON files - Move JsonNodeConverter to utils package
pling-scottlogic
force-pushed
the
VUU70-application-layout-resource
branch
from
ebd3ac3
to
3cfbe20
Compare
pling-scottlogic
requested review from
cfisher-scottlogic and
vferraro-scottlogic
...r/src/test/java/org/finos/vuu/layoutserver/integration/ApplicationLayoutIntegrationTest.java
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Rename header key from 'user' to 'username' - Add integration tests for missing username
pling-scottlogic
requested review from
vferraro-scottlogic and
cfisher-scottlogic
...-server/src/main/java/org/finos/vuu/layoutserver/controller/ApplicationLayoutController.java
Outdated
Show resolved
Hide resolved
...ver/src/test/java/org/finos/vuu/layoutserver/controller/ApplicationLayoutControllerTest.java
Show resolved
Hide resolved
...r/src/test/java/org/finos/vuu/layoutserver/integration/ApplicationLayoutIntegrationTest.java
Outdated
Show resolved
Hide resolved
| Map<String, String> definition = new HashMap<>(); | ||
| definition.put("defKey", "defVal"); |
There was a problem hiding this comment.
How come we're using a stringified JSON object in some requests and a Map in others? My only concern with using a Map is that we're introducing a conversion from Map -> JsonNode which isn't present in the real implementation (unless it is?)
If it is a possible conversion present in the real implementation, I'm all for testing against the various possible types given as inputs, but I would prefer to see that as an explicit test for that purpose so the point of failure is clear, rather than potentially hidden behind a different test scenario.
| persistApplicationLayout(user, initialDefinition); | ||
|
|
||
| String newDefinition = "{\"new-key\": \"new-value\"}"; |
There was a problem hiding this comment.
Do you think it's worth adding an assertion here that the layout exists with this initial form, so we know that the second assertion is checking it's changed and not just currently there? Does that make sense?
assertThat(repository.findById(user).getUsername()).isEqualTo(user);
assertThat(repository.findById(user).getDefinition()).isEqualTo(objectMapper.readTree(newDefinition));| initialDefinition.put("initial-key", "initial-value"); | ||
|
|
||
| persistApplicationLayout(user, initialDefinition); | ||
|
|
There was a problem hiding this comment.
Similar to above, do you think it's worth checking assertThat(repository.findAll()).hasSize(1) prior to deletion so we know the thing has been "deleted"?
|
|
||
| private static ApplicationLayoutRepository mockRepo; | ||
| private static ApplicationLayoutService service; | ||
| private static final DefaultApplicationLayoutLoader defaultLoader = new DefaultApplicationLayoutLoader(); |
There was a problem hiding this comment.
Should DefaultApplicationLayoutLoader be mocked out?
| @Test | ||
| public void createApplicationLayout_invalidDefinition_throwsJsonException() { | ||
| String definition = "invalid JSON"; | ||
|
|
||
| assertThrows(JsonProcessingException.class, () -> | ||
| service.persistApplicationLayout("user", objectMapper.readTree(definition)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Do we even need this test case? I think the controller would reject it before it even reaches this stage? Or is it safer to check for this scenario anyway, regardless of what the caller of this service is doing?
There was a problem hiding this comment.
If we were being super OOPy, I could see us having a FileReader base class and a DefaultApplicationLayoutReader sub class. Whether it's interfaces, abstract, or inheritance. But that's a bit overkill for this simple server. Would you agree?
…UU70-application-layout-resource
| return new ResponseEntity<>(errors.toString(), | ||
| org.springframework.http.HttpStatus.BAD_REQUEST); | ||
| org.springframework.http.HttpStatus.BAD_REQUEST); | ||
| } |
There was a problem hiding this comment.
This should be returning a generated error response
There was a problem hiding this comment.
I am happy to change this to use generateResponse() but note that if we do that we produce the following ErrorResponse:
{ "timestamp": "2023-11-06T15:17:39.657+00:00", "status": 400, "error": "Bad Request", "message": "Validation failed for argument [0] in public org.finos.vuu.layoutserver.dto.response.LayoutResponseDto org.finos.vuu.layoutserver.controller.LayoutController.createLayout(org.finos.vuu.layoutserver.dto.request.LayoutRequestDto): [Field error in object 'layoutRequestDto' on field 'definition': rejected value [null]; codes [NotBlank.layoutRequestDto.definition,NotBlank.definition,NotBlank.java.lang.String,NotBlank]; arguments [org.springframework.context.support.DefaultMessageSourceResolvable: codes [layoutRequestDto.definition,definition]; arguments []; default message [definition]]; default message [Definition must not be blank]] ", "path": "/api/layouts" }
where the errorMessage is
Validation failed for argument [0] in public org.finos.vuu.layoutserver.dto.response.LayoutResponseDto org.finos.vuu.layoutserver.controller.LayoutController.createLayout(org.finos.vuu.layoutserver.dto.request.LayoutRequestDto): [Field error in object 'layoutRequestDto' on field 'definition': rejected value [null]; codes [NotBlank.layoutRequestDto.definition,NotBlank.definition,NotBlank.java.lang.String,NotBlank]; arguments [org.springframework.context.support.DefaultMessageSourceResolvable: codes [layoutRequestDto.definition,definition]; arguments []; default message [definition]]; default message [Definition must not be blank]]
In the other PR we were building the errorMessage inside the handler and the message was looking like this:
[definition: Definition must not be blank]
which is a lot more concise and I liked better but obviously is not ideal because we are not wrapping it in a ErrorResponse like the other 400 responses.
Maybe we can revisit the ErrorResponse constructor at some point to extract the error messages like we were doing in the handler and give us a more concise error message for this error.
There was a problem hiding this comment.
discussed offline: change generateResponse to extract error messages from excpetions
There was a problem hiding this comment.
Just to follow on from what we discussed earlier, I think one of the flaws of the overloading approach is that we could be creating some tight coupling between an 'error handler' and a 'generate response', so each error handler will end up needing it's own 'generate response' method. And in that case, we may as well not have the helper method, and handle each error message generation within the 'error handler'. Making 'generate response' the simple method (by just taking the error message rather than the exception) would resolve this, as we'd still only have one 'error handler', and a single 'generate response'.
Just something I thought of, but I'm happy to hear your thoughts.
There was a problem hiding this comment.
At the moment, all handlers but one share the same logic for extracting the error message from the exception and I like that that is shared (in generateResponse) rather than repeated inside each handler.
As, the only difference between the 2 versions of generateResponse is the way the errorMessages are extracted from the exception, another solution would be to get rid of generateResponse and just have a method that takes the exception and returns the errorMessages (with the same overloading approach) and then we can just return ErrorResponse inside the handlers.
layout-server/src/main/java/org/finos/vuu/layoutserver/exceptions/GlobalExceptionHandler.java
Outdated
Show resolved
Hide resolved
layout-server/src/main/java/org/finos/vuu/layoutserver/controller/LayoutController.java
Outdated
Show resolved
Hide resolved
layout-server/src/main/java/org/finos/vuu/layoutserver/controller/LayoutController.java
Outdated
Show resolved
Hide resolved
layout-server/src/main/java/org/finos/vuu/layoutserver/controller/LayoutController.java
Outdated
Show resolved
Hide resolved
layout-server/src/test/java/org/finos/vuu/layoutserver/controller/LayoutControllerTest.java
Show resolved
Hide resolved
layout-server/src/test/java/org/finos/vuu/layoutserver/controller/LayoutControllerTest.java
Outdated
Show resolved
Hide resolved
layout-server/src/main/java/org/finos/vuu/layoutserver/service/LayoutService.java
Outdated
Show resolved
Hide resolved
layout-server/src/test/java/org/finos/vuu/layoutserver/service/LayoutServiceTest.java
Outdated
Show resolved
Hide resolved
layout-server/src/test/java/org/finos/vuu/layoutserver/service/LayoutServiceTest.java
Show resolved
Hide resolved
layout-server/src/main/java/org/finos/vuu/layoutserver/dto/MetadataDto.java
Outdated
Show resolved
Hide resolved
Description
Adds a resource to the layout server for operating on application layouts. These are top-level descriptions of all the individual layouts currently open for a given user, i.e. all open tabs. These are intended to represent the current visual state of the application for a given user, hence the use of username as primary key/ID.
Change List
Screenshots
Example usage of POST endpoint. Note the username in request headers. The request body is the full application layout definition, which can take the structure of any valid JSON.
Closes #70