Skip to content
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.

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

FileSubmodelElement Endpoints #93

Merged
merged 20 commits into from
Oct 10, 2023
Merged

FileSubmodelElement Endpoints #93

merged 20 commits into from
Oct 10, 2023

Conversation

M-Damm
Copy link
Contributor

@M-Damm M-Damm commented Sep 25, 2023

No description provided.

M-Damm and others added 15 commits August 11, 2023 09:47
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Add Rest API for file upload
Created unit test for file upload

Signed-off-by: Zai Müller-Zhang <[email protected]>
Add json file for test

Signed-off-by: Zai Müller-Zhang <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Add json file for test

Signed-off-by: Zai Müller-Zhang <[email protected]>
Signed-off-by: Zai Müller-Zhang <[email protected]>
Co-authored-by: Danish, Mohammad Ghazanfar Ali<[email protected]>
…recondition failed" if the submodel element is not a file sme.

In InMemorySubmodelRepository, delete unused exception.
Refactor Http Test
# Conflicts:
#	basyx.submodelrepository/basyx.submodelrepository-backend-inmemory/src/main/java/org/eclipse/digitaltwin/basyx/submodelrepository/InMemorySubmodelRepository.java
#	basyx.submodelrepository/basyx.submodelrepository-backend-inmemory/src/test/java/org/eclipse/digitaltwin/basyx/submodelrepository/TestInMemorySubmodelRepository.java
#	basyx.submodelrepository/basyx.submodelrepository-backend-mongodb/src/main/java/org/eclipse/digitaltwin/basyx/submodelrepository/MongoDBSubmodelRepository.java
#	basyx.submodelrepository/basyx.submodelrepository-backend-mongodb/src/main/java/org/eclipse/digitaltwin/basyx/submodelrepository/MongoDBSubmodelRepositoryFactory.java
#	basyx.submodelrepository/basyx.submodelrepository-backend-mongodb/src/test/java/org/eclipse/digitaltwin/basyx/submodelrepository/TestMongoDBSubmodelRepository.java
#	basyx.submodelrepository/basyx.submodelrepository-core/src/main/java/org/eclipse/digitaltwin/basyx/submodelrepository/SubmodelRepository.java
#	basyx.submodelrepository/basyx.submodelrepository-feature-mqtt/src/main/java/org/eclipse/digitaltwin/basyx/submodelrepository/feature/mqtt/MqttSubmodelRepository.java
#	basyx.submodelrepository/basyx.submodelrepository-http/src/main/java/org/eclipse/digitaltwin/basyx/submodelrepository/http/SubmodelRepositoryApiHTTPController.java
#	basyx.submodelrepository/basyx.submodelrepository-http/src/test/java/org/eclipse/digitaltwin/basyx/submodelrepository/http/SubmodelRepositorySubmodelHTTPTestSuite.java
#	basyx.submodelrepository/basyx.submodelrepository-http/src/test/java/org/eclipse/digitaltwin/basyx/submodelrepository/http/TestSubmodelRepositorySubmodelElementsHTTP.java
#	basyx.submodelservice/basyx.submodelservice-backend-inmemory/src/main/java/org/eclipse/digitaltwin/basyx/submodelservice/InMemorySubmodelService.java
#	basyx.submodelservice/basyx.submodelservice-http/src/test/java/org/eclipse/digitaltwin/basyx/submodelservice/http/SubmodelServiceSubmodelElementsTestSuiteHTTP.java
…e default repository name

Signed-off-by: Zai Müller-Zhang <[email protected]>
Copy link
Member

@FischerRene FischerRene left a comment

Choose a reason for hiding this comment

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

Please check if you have used the BaSyx Formatter correctly and address the comments.
If you have any questions, feel free to ask.

Copy link
Member

Choose a reason for hiding this comment

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

What is the benefit of having this class here, if there is not more information inside?

Also missing the copyright notice.

→ I suggest giving this a message at least.

@@ -1,5 +1,6 @@
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
Copy link
Member

Choose a reason for hiding this comment

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

Why has this been split into multiple lines?
→ Wrong formatter settings?

@@ -45,5 +46,13 @@
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter</artifactId>
</dependency>
<dependency>
<groupId>org.apache.tika</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

How might the use of Apache Tika (Apache License 2.0) affect us here?

Copy link
Contributor

Choose a reason for hiding this comment

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

we also use tika in java 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

@FrankSchnicke Is this ok to use this dependency for handling file and directory?

<artifactId>tika-core</artifactId>
</dependency>
<dependency>
<groupId>commons-io</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

Same question regarding also the licensing.

.ifPresent(id -> {
throw new CollidingIdentifierException(id);
});
submodelsToCheck.stream().map(submodel -> submodel.getId()).filter(id -> !ids.add(id)).findAny().ifPresent(id -> {
Copy link
Member

Choose a reason for hiding this comment

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

No change to the logic. Might be formatter issue in this file again.

Copy link
Contributor

Choose a reason for hiding this comment

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

The basyx formatter format these lines into one line. Should we revert this change back or should we insist on basyx format?

Copy link
Contributor

Choose a reason for hiding this comment

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

solved

Copy link
Member

Choose a reason for hiding this comment

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

JavaDoc for public methods missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

resolved

}

@Test
public void deleteFile() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this test throw an IOException?

Copy link
Contributor

Choose a reason for hiding this comment

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

the method getInputStreamOfFileFromClasspath("SampleJsonFile.json") in this test throws IOException

Copy link
Contributor

Choose a reason for hiding this comment

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

resolved

Copy link
Member

Choose a reason for hiding this comment

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

Testcase for updating a file and checking if that has worked is missing.
→ It's part of the API with setFileValue, so I'd expect an "update" check as well, even if it (right now) has the logic of deleting+creating it again.

Copy link
Contributor

Choose a reason for hiding this comment

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

resolved

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add another test case for corrupt files?

What would happen if I try to upload a PDF as an image file for example?

Copy link
Contributor

Choose a reason for hiding this comment

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

the user takes responsibility for this.

Copy link
Member

Choose a reason for hiding this comment

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

Why were these changes made?

Especially the return does not seem to make any difference to me.

zhangzai123 and others added 5 commits October 6, 2023 15:09
Signed-off-by: Zai Müller-Zhang <[email protected]>
Signed-off-by: Zai Müller-Zhang <[email protected]>
- Minor refactoring

Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Copy link
Contributor

@mdanish98 mdanish98 left a comment

Choose a reason for hiding this comment

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

Approved

@FrankSchnicke FrankSchnicke merged commit 07fa3d7 into eclipse-basyx:main Oct 10, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants