-
Notifications
You must be signed in to change notification settings - Fork 49
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
base: main
Are you sure you want to change the base?
Conversation
I will integrate the mongoDB solution |
MongoDB is integrated and the java method delegation is implemented as feature now. |
Hi @geso02 , Thanks a lot for your contribution. |
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; | ||
import org.springframework.boot.context.properties.ConfigurationProperties; | ||
import org.springframework.stereotype.Component; | ||
/** |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
@@ -0,0 +1,27 @@ | |||
basyx: | |||
backend: InMemory |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 | |||
|
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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). |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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:
Lines 236 to 244 in 935edab
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.
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
andbasyx.submodelservice.component/example
folders for further details.Closes #445