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

Provide a generic submodel service component #446

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

geso02
Copy link
Contributor

@geso02 geso02 commented Sep 11, 2024

This implementation addresses the feature request #445.

Test files are included in this commit, and an example folder illustrates the setup.

Please refer to the README files in the basyx.submodelservice.component and basyx.submodelservice.component/example folders for further details.

Closes #445

@geso02
Copy link
Contributor Author

geso02 commented Sep 17, 2024

I will integrate the mongoDB solution

@geso02 geso02 closed this Sep 17, 2024
@geso02 geso02 reopened this Sep 19, 2024
@geso02
Copy link
Contributor Author

geso02 commented Sep 19, 2024

MongoDB is integrated and the java method delegation is implemented as feature now.

@geso02 geso02 requested a review from mdanish98 September 19, 2024 09:00
@mdanish98
Copy link
Contributor

Hi @geso02 ,

Thanks a lot for your contribution.
We have added this PR for review in our pipeline. We will update this PR once we are finished with the review.

@aaronzi aaronzi requested review from zhangzai123 and removed request for mdanish98 October 31, 2024 08:22
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.stereotype.Component;
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Please insert comment about the functionality of this class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the class OperationDispatcherMapping, I believe that additional comments may not be strictly necessary due to the clarity provided by the Spring annotations used:

@component makes it clear that this class is a Spring-managed bean.
@ConfigurationProperties explains how the configuration is mapped from application properties to the class fields.
@ConditionalOnProperty indicates that this bean is only active if the corresponding feature is enabled.
These annotations already serve as inline documentation, making the purpose and behavior of the class quite evident. Furthermore, the configuration and usage of this feature are detailed in the project's README.

However, if you feel a comment would still improve the code, I can propose something concise to capture the intent:

// This class maps configuration properties for the "Operation Dispatching Submodel Service Feature."
// It enables dynamic operation mappings when the feature is activated via application properties.
// Configurations:
// - defaultMapping: A fallback mapping used when no specific mapping is found.
// - mappings: A map of specific operation names to their mappings, configured in application.yml or application.properties.

Let me know if this aligns with your expectations or if additional context is needed!

Copy link
Member

Choose a reason for hiding this comment

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

Hi @geso02,

For me it would be fine if you keep the code as is without adding the comments.
Nevertheless, it would be great if you could also create a PR for the basyx-wiki with documentation for this feature.

In addition to that, could you please also build an example for this feature and put it here.

Thank you very much for your PR, this will be a great addition to BaSyx!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will take a look at it.

@mateusmolina-iese mateusmolina-iese self-requested a review December 6, 2024 13:31
@@ -0,0 +1,27 @@
basyx:
backend: InMemory
Copy link
Member

Choose a reason for hiding this comment

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

Should be MongoDB

File submodelFile = new File(filePath);
try (FileInputStream fIn = new FileInputStream(submodelFile);
BufferedInputStream bIn = new BufferedInputStream(fIn)) {
JsonDeserializer deserializer = new JsonDeserializer();
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding support to Submodels of other types

WORKDIR /application
ARG JAR_FILE=target/*-exec.jar
COPY ${JAR_FILE} basyxExecutable.jar
ARG AAS4J_JAR_FILE=target/libs/aas4j-model-1.0.2.jar
Copy link
Member

Choose a reason for hiding this comment

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

aas4j version is now at 1.0.3. Consider a way to fetch this value automatically from the main pom

@@ -0,0 +1,19 @@
## Submodel Service Component Example

Copy link
Member

Choose a reason for hiding this comment

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

Anyway to avoid commiting the .jar binary?

/**
* @author Gerhard Sonnenberg DFKI GmbH
*/
public interface OperationExecutor {
Copy link
Member

Choose a reason for hiding this comment

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

Consider using InvokableOperation and Invokable

@@ -0,0 +1,5 @@
FROM eclipsebasyx/submodel-service:0.2.0-SNAPSHOT
Copy link
Member

Choose a reason for hiding this comment

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

2.0.0-SNAPSHOT?

fi
fi

docker-compose up --build --force-recreate --detach --wait
Copy link
Member

Choose a reason for hiding this comment

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

docker compose

docker-compose is apparently not being recognized anymore in some distros with default docker installation (Ubuntu 22.04)
Same for line 96

@@ -0,0 +1,99 @@
# Eclipse BaSyx - Standalone Submodel Service Component
Copy link
Member

Choose a reason for hiding this comment

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

general remark: this PR introduces two features: the SmService Component and the OperationDispatching. I would suggest rewriting the PR's title to mention the OperationDispatching feature or splitting the PR. I don't think 'Provide a generic submodel service component' summarizes the full scope of the changes.


The [docker-compose.yml](docker-compose.yml) file provides a basic setup for starting the service. The Docker image used is initially built based on the [Dockerfile located in the parent directory](../Dockerfile).

Volumes are used to provide the Submodel and the executable source code to the container, which are referenced in the environment section. The mapping of `idShortPath` to Java classes is also referenced there and loaded via [application-mappings.yml](application-mappings.yml). Alternatively, you can simplify the setup by configuring everything directly in [application.yml](application.yml).
Copy link
Member

@mateusmolina-iese mateusmolina-iese Dec 9, 2024

Choose a reason for hiding this comment

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

I found this paragraph a bit confusing. The example/application.yml is not being used by the example/docker-compose.yml or in any of the integration tests. I understand it's there to serve as an example for a full configuration.

To improve understandability and clarity, the standalone example could be moved to the ./examples folder (root dir). I know some of the files in here are being used for the integration tests; consider refactoring the package such, that the integration files are separated from the example itself and the full application.yml is used in the example.

* @author Gerhard Sonnenberg DFKI GmbH
*/
@FunctionalInterface
public interface OperationExecutorProvider {
Copy link
Member

Choose a reason for hiding this comment

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

One idea would be to add the InvokableOperations to the Submodel when loading it via the GenericSubmodelFactory, instead of searching for the corresponding idShortPath in the OperationDispatcherSubmodelService.

This way no new feature is needed and we rely on the same mechanism already implemented in the Impl of AasService:

public OperationVariable[] invokeOperation(String idShortPath, OperationVariable[] input) {
SubmodelElement sme = getSubmodelElement(idShortPath);
if (!(sme instanceof InvokableOperation))
throw new NotInvokableException(idShortPath);
InvokableOperation operation = (InvokableOperation) sme;
return operation.invoke(input);
}

Let me know if this idea helps.

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.

[FEATURE] Support for Submodel Services with Configurable Docker Image
5 participants