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

Opm plcentitymanager #29

Merged
merged 30 commits into from
Oct 25, 2018
Merged

Opm plcentitymanager #29

merged 30 commits into from
Oct 25, 2018

Conversation

JulianFeinauer
Copy link
Contributor

I just finished the first (working) implementation of the Object-Plc-Mapping (OPM).
This is a JPA Like functionality on top of plc4x which allows to issue requests against plc4x by calling Methods on POJOs!

Basically, it uses 3 classes.

  • PlcEntityManager: This is the main class. A call on read(…) or connect(…) does all the necessary magic
  • PlcEntity (Annotation): A Pojo needs this annotation to be used with read(…) or connect(…) above
  • PlcField (Annotation): Fields in the Pojo that are annotated with this field are fetched from the plc

Some simple tests (with mocking) can be found in PlcEntityManagerTest.

I would be really happy for some feedback on the basic semantics (which annotations, where to put them, …) or even some testing.
If the feedback on this is positive I would like issue a PR for this.

- updated copyrights
- removed @author
- fixed several small issues
- added documentation
# Conflicts:
#	plc4j/utils/pom.xml
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
public @interface PlcEntity {
String value();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can add a additional String connectionUrl() method that is semantical the same as value() but haven't found a elegant way to achieve this.


try (PlcConnection connection = driverManager.getConnection(source)) {

if (!connection.getReader().isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could be replaced with orThrow()


// Do the necessary queries for all fields
// HashMap<ReadRequestItem<?>, Field> requestItems = new HashMap<>();
for (Field field : clazz.getDeclaredFields()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

later on we might support method injection..

// Perform the request
PlcReadResponse<?> response;
try {
// TODO: make configurable.
Copy link
Contributor

Choose a reason for hiding this comment

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

added a todo

* @return a connected entity.
* @throws OPMException when proxy can't be build.
*/
public <T> T connect(Class<T> clazz) throws OPMException {
Copy link
Contributor

Choose a reason for hiding this comment

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

confuses me a bit. Is this the same as merge in jpa?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its kinda similar to find(...) in JPA because it returns a connected entity.
But as it does not have a primary key (makes no sense) I decided to name it connect.
But thinking about it, it is pretty similar to merge(...) with the difference that it takes a class and not an instance.
But I'm open for changes in the naming.

public Object intercept(@This Object o, @Origin Method m, @SuperCall Callable<?> c, @Super Object that) throws OPMException {
LOGGER.trace("Invoked method {} on connected PlcEntity {}", m.getName(), that);

if (m.getName().startsWith("get")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should assert that no method starting with get and having attributes get picked up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a fair point. And in fact, we should also support isXXX() for boolean fields.
We should add a Todo here, or better extract this in a method isGetter(m) where we can adopt all these conditions.

PlcReader plcReader = connection.getReader().get();

// Build the query
PlcReadRequest.Builder builder = plcReader.readRequestBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

so currently only read is possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, perhaps I didnt state this enough. Currently, only read. It should not be that big of a deal to add writing. I'll open a Jira for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.. is PLC4X-70

}
}

// TODO: why isn't o used?
Copy link
Contributor

Choose a reason for hiding this comment

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

added a TODO

try {
field.set(o, getTyped(field.getType(), response, fieldName));
} catch (ClassCastException e) {
// TODO should we simply fail here?
Copy link
Contributor

Choose a reason for hiding this comment

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

think so as this can be checked beforehand and so an simple fail is sufficient here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory this should be an IllegalStateException, as this should never happen, or?

@sruehl
Copy link
Contributor

sruehl commented Oct 25, 2018

looks good to me :)

@asfgit asfgit merged commit 778b05b into master Oct 25, 2018
@sruehl sruehl deleted the opm-plcentitymanager branch October 25, 2018 16:01
@JulianFeinauer
Copy link
Contributor Author

Thank you @sruehl for the comments.
I answered your comments although it is already merged. I would love to test this feature (together with your pool).

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.

None yet

3 participants