Fixes a error in the assetIds Query Parameter of the /shells endpoint #771
Conversation
FriedJannik
force-pushed
the
fix/assetIdsQuery
branch
from
8d181d7 to
4d970ce
Compare
|
Is this PR still being worked on? If I remember correctly, multiple |
|
Hi @arnoweiss, we are working on this again - there is a mistake in the filtering of the assertIds in the code which throws away every globalAssetId except the first one. |
|
|
||
| List<AssetAdministrationShell> shells = results.getMappedResults(); | ||
|
|
||
| String nextCursor = shells.isEmpty() |
There was a problem hiding this comment.
Just out of curiosity - in what scenario could there be multiple globalAssetId's? The only scenario I could think of is if a globalAssetId is set (string) and a specificAssetId.name is equal to globalAssetId. Is that the case that's handled here?
There was a problem hiding this comment.
If you provide an assetId with name globalAssetId it will always be interpreted as globalAssetId. Therefore this mistake will only happen if two globalAssetIds are intentionally provided via the request
There was a problem hiding this comment.
This is a fix that correctly handles the case that two globalAssetIds are provided (even tho this should not happen in the first place)
There was a problem hiding this comment.
Pull request overview
This PR fixes an error in the assetIds query parameter handling for the /shells endpoint where multiple globalAssetId values could be specified but the second one would be ignored or cause incorrect behavior. The fix ensures that when multiple globalAssetIds are provided, an empty result is returned (since an AAS can only have one globalAssetId).
- Changed MongoDB backend to collect all globalAssetIds into a list instead of just finding the first one
- Changed InMemory backend to detect multiple globalAssetIds and return empty results
- Added HTTP integration test to verify correct behavior with multiple globalAssetIds
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| MongoDBAasOperations.java | Modified to collect all globalAssetIds into a list and add each as a filter criteria |
| InMemoryAasBackend.java | Added early return check to detect multiple globalAssetIds and return empty results |
| EmptyResponse.json | Added test resource file for expected empty response format |
| AasRepositoryHTTPSuite.java | Added integration test for multiple globalAssetIds scenario |
| .gitignore | Added .DS_Store to ignore macOS system files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (SpecificAssetId globalAssetId : globalAssetIds) { | ||
| criteriaList.add(Criteria.where("assetInformation.globalAssetId").is(globalAssetId.getValue())); | ||
| } |
There was a problem hiding this comment.
The logic for handling multiple globalAssetIds is incorrect. When multiple globalAssetIds are provided, adding each as a separate criteria with AND operator (line 217) means the query would require an AAS to match ALL globalAssetIds simultaneously, which is impossible since an AAS can only have one globalAssetId. This should either return an empty result (like the InMemory implementation) or use OR logic. The InMemory backend correctly returns an empty list when multiple globalAssetIds are detected.
| if(assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).count() > 1){ | ||
| return filteredAas; | ||
| } | ||
| globalAssetId = assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).findFirst().get().getValue(); |
There was a problem hiding this comment.
The stream is being traversed twice unnecessarily. The first traversal counts globalAssetId entries, and line 123 traverses the stream again to find the first one. Consider storing the filtered list once and checking its size to avoid redundant stream operations.
| if(assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).count() > 1){ | |
| return filteredAas; | |
| } | |
| globalAssetId = assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).findFirst().get().getValue(); | |
| List<SpecificAssetId> globalAssetIdList = assetIds.stream() | |
| .filter(assetId -> assetId.getName().equals("globalAssetId")) | |
| .collect(Collectors.toList()); | |
| if(globalAssetIdList.size() > 1){ | |
| return filteredAas; | |
| } | |
| if(!globalAssetIdList.isEmpty()) { | |
| globalAssetId = globalAssetIdList.get(0).getValue(); | |
| } |
| List<AssetAdministrationShell> filteredAas = new java.util.ArrayList<>(); | ||
| String globalAssetId = null; | ||
| try { | ||
| if(assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).count() > 1){ |
There was a problem hiding this comment.
Missing spaces around the condition inside the if statement. According to Java code style conventions, there should be a space after the opening parenthesis and before the closing parenthesis, or at minimum consistent formatting with the rest of the codebase.
| if(assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).count() > 1){ | |
| if (assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).count() > 1) { |
| } | ||
|
|
||
| @Test | ||
| public void getAllAasWithMultipleAssetIds() throws IOException, ParseException { |
There was a problem hiding this comment.
The test method name "getAllAasWithMultipleAssetIds" is ambiguous. It doesn't clearly indicate that it's specifically testing multiple globalAssetIds (not multiple specificAssetIds). Consider renaming to "getAllAasWithMultipleGlobalAssetIds" to better reflect what is being tested.
| public void getAllAasWithMultipleAssetIds() throws IOException, ParseException { | |
| public void getAllAasWithMultipleGlobalAssetIds() throws IOException, ParseException { |
3 participants
Description of Changes
This PR Fixes a error in the assetIds Query Parameter of the /shells endpoint where it was possible to specify a 2nd globalAssetId that got ignored
Related Issue
BaSyx Configuration for Testing
AAS Files Used for Testing
Additional Information