[rest] Fix GET /rest/persistence/items returns null entries - #5387
[rest] Fix GET /rest/persistence/items returns null entries#5387florian-h05 wants to merge 3 commits into
/rest/persistence/items returns null entries#5387Conversation
…ntries When querying Items from InMemory, I noticed that the return is an array of null values. This was caused by a silent serialization failure, where Gson serialized anonymous implementations of PersistenceItemInfo to null due to missing fields. Serialization was error-prone in general, as the serialization results depended on the field names of PersistenceItemInfo implementations from the persistence services. This PR fixes that issue by introducing a proper DTO and mapping to that. It also adds unit tests. Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Florian Hotze <dev@florianhotze.com>
florian-h05
changed the title
/rest/persistence/items?serviceid=... returns null entries/rest/persistence/items returns null entries
|
@florian-h05 I had seen similar issues of fields not being included, but had not seen the consequence of having items without item name. That's most likely specific to InMemoryPeristence because it creates the item name through a method call and doesn't have a field for it. The services I tested have a field that gets serialized. Anyway, I also solved it in #5336, in a very similar way, creating a record PersistenceItemInfoDTO, embedded in the PersistenceResource class. As it is only used there, I didn't extract it in a separate class. |
|
Sorry, didn't notice it fixed the issue, only read the PR description and wanted to look at the code later. |
|
BTW, when applying aliases, I think the issue would have occurred for all services as the PersistenceResource then used an anonymous implementation of the PersistenceItemInfo interface as well. |
Yes, correct. That would have lead to the same issue. |
Signed-off-by: Florian Hotze <dev@florianhotze.com>
|
Marked as draft in favor of merging #5336, which also includes this fix. |
| } | ||
|
|
||
| public PersistenceItemInfoDTO(PersistenceItemInfo info, String alias) { | ||
| this.name = alias; |
There was a problem hiding this comment.
I don't think you should do this. When using getPersistenceItemInfo, it should return the item names as stored in the storage, and all of them.
If you map back what you get from the storage to the configured alias, you will never be able to see what is in the persistence storage that does not map with an item name or configured alias.
There was a problem hiding this comment.
I've only transferred the logic as it was previously in PersistenceResource, but having it here can be misleading. Anyway, I want to close this PR in favor of your PR, so what's here doesn't really matter.
| PersistenceServiceConfiguration config = persistenceServiceConfigurationRegistry.get(serviceId); | ||
| Map<String, String> aliases = config != null ? config.getAliases() : Map.of(); | ||
| return service.getItemInfo().stream().map(info -> { | ||
| String alias = aliases.get(info.getName()); |
There was a problem hiding this comment.
Don't try to map back to aliases here. Only the caller should do that.
This was another bug in this code by the way, which made the API inconsistent with the documentation.
2 participants
When querying Items from InMemory, I noticed that the return value from GET
/rest/persistence/items?serviceid=inmemoryis an array of null values.This was caused by a silent serialization failure, where Gson serialized anonymous implementations of PersistenceItemInfo to null due to missing fields. Serialization was error-prone in general, as the serialization results depended on the field names of PersistenceItemInfo implementations from the persistence services.
This PR fixes that issue by introducing a proper DTO and mapping to that. It also adds unit tests.