Skip to content

Fixes a error in the assetIds Query Parameter of the /shells endpoint #771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aaronzi merged 5 commits into from
Dec 11, 2025
+56 −11
Merged

Fixes a error in the assetIds Query Parameter of the /shells endpoint #771

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d970ce
Fixes /shells Endpoint Query Param
FriedJannik Jun 12, 2025
94ec98b
Merge remote-tracking branch 'basyx/main' into fix/assetIdsQuery
FriedJannik Dec 11, 2025
8d64fd8
Intermediate commit
FriedJannik Dec 11, 2025
8395bc0
Fixes InMemory Implementation of filtering
FriedJannik Dec 11, 2025
8d85804
Fixes InMemory Implementation of filtering
FriedJannik Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,5 @@ local.properties
/.env

**.iml

.DS_Store
18 changes: 18 additions & 0 deletions ...rc/test/java/org/eclipse/digitaltwin/basyx/aasrepository/http/AasRepositoryHTTPSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,16 @@ public void getAllAasWithIdShort() throws IOException, ParseException {
BaSyxHttpTestUtils.assertSameJSONContent(getPaginatedAas1JSONString(), getJSONWithoutCursorInfo(actualJsonFromServer));
}

@Test
public void getAllAasWithMultipleAssetIds() throws IOException, ParseException {
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
public void getAllAasWithMultipleAssetIds() throws IOException, ParseException {
public void getAllAasWithMultipleGlobalAssetIds() throws IOException, ParseException {

Copilot uses AI. Check for mistakes.
createMultipleAasOnServer();
CloseableHttpResponse retrievalResponse = getAllAasMultipleGlobalAssetIdsParam();
assertEquals(HttpStatus.OK.value(), retrievalResponse.getCode());

String actualJsonFromServer = BaSyxHttpTestUtils.getResponseAsString(retrievalResponse);
BaSyxHttpTestUtils.assertSameJSONContent(getEmptyResultJSONString(), getJSONWithoutCursorInfo(actualJsonFromServer));
}

@Test
public void deleteAas() throws IOException {
createDummyAasOnServer(getAas1JSONString());
Expand Down Expand Up @@ -423,6 +433,10 @@ public void deleteNonExistingThumbnail() throws FileNotFoundException, Unsupport
private String getPaginatedAas1JSONString() throws FileNotFoundException, IOException {
return BaSyxHttpTestUtils.readJSONStringFromClasspath("PaginatedAasSimple_1.json");
}

private String getEmptyResultJSONString() throws FileNotFoundException, IOException {
return BaSyxHttpTestUtils.readJSONStringFromClasspath("EmptyResponse.json");
}

private String getJSONWithoutCursorInfo(String response) throws JsonMappingException, JsonProcessingException {
return BaSyxHttpTestUtils.removeCursorFromJSON(response);
Expand Down Expand Up @@ -489,6 +503,10 @@ protected CloseableHttpResponse getAllAasGlobalAssetIdsParam() throws IOExceptio
return BaSyxHttpTestUtils.executeGetOnURL(getURL()+"?assetIds=ew0KIm5hbWUiOiJnbG9iYWxBc3NldElkIiwNCiJ2YWx1ZSI6Imdsb2JhbEFzc2V0SWQiDQp9");
}

protected CloseableHttpResponse getAllAasMultipleGlobalAssetIdsParam() throws IOException {
return BaSyxHttpTestUtils.executeGetOnURL(getURL()+"?assetIds=ew0KIm5hbWUiOiJnbG9iYWxBc3NldElkIiwNCiJ2YWx1ZSI6Imdsb2JhbEFzc2V0SWQiDQp9&assetIds=ew0KIm5hbWUiOiJnbG9iYWxBc3NldElkIiwNCiJ2YWx1ZSI6ImR1bW15QWFzQXNzZXRJZCINCn0");
}

protected CloseableHttpResponse getAllAasIdShortParam() throws IOException {
return BaSyxHttpTestUtils.executeGetOnURL(getURL()+"?idShort=ExampleMotor");
}
Expand Down
5 changes: 5 additions & 0 deletions basyx.aasrepository/basyx.aasrepository-http/src/test/resources/EmptyResponse.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"paging_metadata": {},
"result": [
]
}
3 changes: 3 additions & 0 deletions ...ry/src/main/java/org/eclipse/digitaltwin/basyx/aasservice/backend/InMemoryAasBackend.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ public Iterable<AssetAdministrationShell> getAllAas(List<SpecificAssetId> assetI
List<AssetAdministrationShell> filteredAas = new java.util.ArrayList<>();
String globalAssetId = null;
try {
if(assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).count() > 1){
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if(assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).count() > 1){
if (assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).count() > 1) {

Copilot uses AI. Check for mistakes.
return filteredAas;
}
globalAssetId = assetIds.stream().filter(assetId -> assetId.getName().equals("globalAssetId")).findFirst().get().getValue();
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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();
}

Copilot uses AI. Check for mistakes.
assetIds = assetIds.stream().filter(assetId -> !assetId.getName().equals("globalAssetId")).collect(Collectors.toList());
} catch (Exception e) {}
Expand Down
12 changes: 5 additions & 7 deletions .../src/main/java/org/eclipse/digitaltwin/basyx/aasservice/backend/MongoDBAasOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,11 @@ private List<Criteria> buildAasFilterCriteria(List<SpecificAssetId> assetIds, St
List<Criteria> criteriaList = new ArrayList<>();

// Extract globalAssetId from assetIds
String globalAssetId = null;
List<SpecificAssetId> globalAssetIds = new ArrayList<>();
try {
globalAssetId = assetIds.stream()
globalAssetIds = assetIds.stream()
.filter(assetId -> "globalAssetId".equals(assetId.getName()))
.findFirst()
.map(SpecificAssetId::getValue)
.orElse(null);
.toList();
Copy link

@arnoweiss arnoweiss Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@aaronzi aaronzi Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@aaronzi aaronzi Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fix that correctly handles the case that two globalAssetIds are provided (even tho this should not happen in the first place)


assetIds = assetIds.stream()
.filter(assetId -> !"globalAssetId".equals(assetId.getName()))
Expand All @@ -267,8 +265,8 @@ private List<Criteria> buildAasFilterCriteria(List<SpecificAssetId> assetIds, St
}

// Match globalAssetId if present
if (globalAssetId != null && !globalAssetId.isEmpty()) {
criteriaList.add(Criteria.where("assetInformation.globalAssetId").is(globalAssetId));
for (SpecificAssetId globalAssetId : globalAssetIds) {
criteriaList.add(Criteria.where("assetInformation.globalAssetId").is(globalAssetId.getValue()));
}
Comment on lines +268 to 270
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

return criteriaList;
Expand Down