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

Feat database new #273

Closed
wants to merge 13 commits into from
Closed

Feat database new #273

wants to merge 13 commits into from

Conversation

jantischhoefer
Copy link
Member

@jantischhoefer jantischhoefer commented May 5, 2021

Summary

Improvements to the current database implementation.

Details

Add implementation for querying data periodically from db.
Supply an example for project specific usage.

Additional information

There is a problem between PeriodicDataPublisher.java and MongoDatabaseService.java, when handling the reply message from the database to the publisher. I assume the problem is coming from a faulty usage of the JacksonCodec.

CLA

  • I have signed the individual contributor's license agreement and sent it to the board of the WüSpace e. V. organization.

Comments and documentation follow tomorrow. Reply handling is still an issue.
@jantischhoefer
Copy link
Member Author

Current error message to commit 45fc7b0:

java.lang.IllegalArgumentException: Unrecognized field "datetime" (class io.vertx.core.json.JsonObject), not marked as ignorable (one known property: "map"])
 at [Source: UNKNOWN; line: -1, column: -1] (through reference chain: de.wuespace.telestion.services.database.DbRequest["query"]->io.vertx.core.json.JsonObject["datetime"])
	at com.fasterxml.jackson.databind.ObjectMapper._convert(ObjectMapper.java:4312)
	at com.fasterxml.jackson.databind.ObjectMapper.convertValue(ObjectMapper.java:4243)
	at io.vertx.core.json.jackson.DatabindCodec.fromValue(DatabindCodec.java:84)
	at io.vertx.core.json.JsonObject.mapTo(JsonObject.java:119)
	at de.wuespace.telestion.api.message.JsonMessage.on(JsonMessage.java:40)
	at de.wuespace.telestion.api.message.JsonMessage.on(JsonMessage.java:62)
	at de.wuespace.telestion.services.database.MongoDatabaseService.lambda$registerConsumers$3(MongoDatabaseService.java:79)
	at io.vertx.core.impl.EventLoopContext.emit(EventLoopContext.java:52)
	at io.vertx.core.impl.DuplicatedContext.emit(DuplicatedContext.java:194)
	at io.vertx.core.eventbus.impl.MessageConsumerImpl.dispatch(MessageConsumerImpl.java:177)
	at io.vertx.core.eventbus.impl.HandlerRegistration$InboundDeliveryContext.next(HandlerRegistration.java:163)
	at de.wuespace.telestion.services.monitoring.MessageLogger.lambda$start$1(MessageLogger.java:37)
	at io.vertx.core.eventbus.impl.HandlerRegistration$InboundDeliveryContext.next(HandlerRegistration.java:142)
	at io.vertx.core.eventbus.impl.HandlerRegistration$InboundDeliveryContext.dispatch(HandlerRegistration.java:128)
	at io.vertx.core.impl.AbstractContext.dispatch(AbstractContext.java:107)
	at io.vertx.core.eventbus.impl.HandlerRegistration.dispatch(HandlerRegistration.java:104)
	at io.vertx.core.eventbus.impl.MessageConsumerImpl.deliver(MessageConsumerImpl.java:183)
	at io.vertx.core.eventbus.impl.MessageConsumerImpl.doReceive(MessageConsumerImpl.java:168)
	at io.vertx.core.eventbus.impl.HandlerRegistration.lambda$receive$0(HandlerRegistration.java:54)
	at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:164)
	at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:472)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:500)
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:831)
Caused by: com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "datetime" (class io.vertx.core.json.JsonObject), not marked as ignorable (one known property: "map"])
 at [Source: UNKNOWN; line: -1, column: -1] (through reference chain: de.wuespace.telestion.services.database.DbRequest["query"]->io.vertx.core.json.JsonObject["datetime"])
	at com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException.from(UnrecognizedPropertyException.java:61)
	at com.fasterxml.jackson.databind.DeserializationContext.handleUnknownProperty(DeserializationContext.java:989)
	at com.fasterxml.jackson.databind.deser.std.StdDeserializer.handleUnknownProperty(StdDeserializer.java:1965)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownProperty(BeanDeserializerBase.java:1686)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownVanilla(BeanDeserializerBase.java:1664)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:330)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:187)
	at com.fasterxml.jackson.databind.deser.SettableBeanProperty.deserialize(SettableBeanProperty.java:542)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeWithErrorWrapping(BeanDeserializer.java:565)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:449)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1390)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:362)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:195)
	at com.fasterxml.jackson.databind.ObjectMapper._convert(ObjectMapper.java:4307)
	... 25 common frames omitted

Problem is MongoDB answers a .find(...) request with a List<JsonObject>. This can't be send back over the eventbus, because there is no codec for List<JsonObject>. Therefore, i wrapped the reply in a JsonArray, which works fine. But there is a problem when deserializing the message, because MongoDB adds two keys to the reply object ("_id" and "datetime").

@jantischhoefer
Copy link
Member Author

jantischhoefer commented May 6, 2021

Maybe something like this in JacksonCodec is needed:

  1. Ignoring Unknown Properties
    Sometimes the JSON you are trying to read might include some properties not defined in the Java class. Maybe the JSON was updated but the change is not yet reflected in the Java class. In such cases, you might end up with an exception like this:
Exception in thread "main" com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "gender" (class sample.User), not marked as ignorable (6 known properties: ...)

You can tell Jackson to ignore such properties on a global level by disabling a deserialization feature as follows:

mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);

Alternatively, you can use the following annotation on the class to ignore unknown properties.

@JsonIgnoreProperties(ignoreUnknown=true)
public class User {
    ...
}

https://www.novixys.com/blog/jackson-json-serialization/

@cb0s
Copy link
Member

cb0s commented May 6, 2021

I just looked over your code and a question arose: Are these D2-specific code sections for testing purposes? Because if not so, we have to find a way to outsource them somehow.

Alternatively, you can use the following annotation on the class to ignore unknown properties.

@JsonIgnoreProperties(ignoreUnknown=true)
public class User {
    ...
}

I think I would like this solution with an annotation. You don't change the behaviour on a global level but get it working.

Copy link
Member

@cb0s cb0s left a comment

Choose a reason for hiding this comment

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

In general this looks quite promising. However I would like to see the D2-parts gone before merging it, ok? :)

@jantischhoefer
Copy link
Member Author

jantischhoefer commented May 6, 2021

In general this looks quite promising. However I would like to see the D2-parts gone before merging it, ok? :)

Good. Before merging, i just want to fix the request-reply issue. I couldn't find a working solution for this yet.
Which D2-parts do you mean specifically? I thought i moved everything D2-specific into telestion-example.

@jantischhoefer
Copy link
Member Author

I just looked over your code and a question arose: Are these D2-specific code sections for testing purposes? Because if not so, we have to find a way to outsource them somehow.

Alternatively, you can use the following annotation on the class to ignore unknown properties.

@JsonIgnoreProperties(ignoreUnknown=true)
public class User {
    ...
}

I think I would like this solution with an annotation. You don't change the behaviour on a global level but get it working.

Do you have an idea where to put the annotation?

@jantischhoefer
Copy link
Member Author

It looks like he wants to map the received reply from the database, back to the Java-Record DbRequest.
de.wuespace.telestion.services.database.DbRequest["query"]->io.vertx.core.json.JsonObject["datetime"])

@cb0s @jvpichowski do you know if the replied JsonObject must have the same structure as the requested JsonObject sent by vertx.eventbus().request(...)?

The problem with the @JsonIgnoreProperties(ignoreUnknown=true) is that i cannot apply that to JsonObject`.

@jantischhoefer
Copy link
Member Author

I think i found the source of the thrown Exception.

In the PeriodicDataPublisher.java i put a new key ("datetime") to the DbRequest.query()field which is a JsonObject, in order to query any data greater than the last published date.

private DbRequest dbRequest;
...
private void databaseRequest() {
...
   var dateQuery = new JsonObject().put("$gte", new JsonObject().put("$date", timeOfLastDataSet));
   dbRequest.query().put("datetime", dateQuery);
... 
}

Seems like for jackson to work properly it has to know the added key to the JsonObject pre conversion.

@jvpichowski @cb0s do you have any idea how to solve this issue?

In principle the DbRequest.query() should accept any key/value-pair, because that's how queries in MongoDB work.

@jantischhoefer
Copy link
Member Author

With the recent commit 5a54ed3 i switched the implementation of DbResponsefrom JsonObject to String.
The String is then converted to a JsonObject query for MongoDB in MongoDatabaseService.java.
This works fine now, but i have to add input validation, etc. in the near future to prevent possibly thrown exceptions.

Copy link
Member

@cb0s cb0s left a comment

Choose a reason for hiding this comment

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

Looks good to me so far.

private static record Configuration(@JsonProperty JsonObject dbConfig, @JsonProperty String dbPoolName) {
private Configuration() {
this(new JsonObject().put("db_name", "raketenpraktikum").put("useObjectId", true), "raketenpraktikumPool");
this(new JsonObject().put("db_name", "daedalus2").put("useObjectId", true), "d2Pool");
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason why you have daedalus2 in the private constructor? Not that it really matters, however for keeping this repository "client-free" you maybe could consider to unit the Configuration with null values

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you're right. That still comes from my perception, that the private constructor is used, when no values are specified.

@jantischhoefer
Copy link
Member Author

I just moved the currently working database implementation from the daedalus2-project back to telestion-core.
This PR can be merged, if you are fine with that @cb0s & @jvpichowski?
For cleanup purposes i would like to delete the old feat-database branch, because it is massively behind and not important anymore. If this PR is merged i will improve the current database implementation in a new branch.

@jantischhoefer jantischhoefer marked this pull request as ready for review May 15, 2021 14:25
@jvpichowski
Copy link
Member

Do it if there are no D2 dependencies. :)

@jantischhoefer
Copy link
Member Author

I cannot merge by myself, because merging is blocked and can only be performed automatically with 1 approving review. @jvpichowski

@fussel178 fussel178 requested a review from cb0s May 16, 2021 12:51
@fussel178 fussel178 marked this pull request as draft July 7, 2021 21:04
@fussel178 fussel178 added 💣 breaking Issue or PR that includes breaking changes 🌷 enhancement New feature or request 📄 java Pull requests that update Java code 🎩 telestion-services Everything related to the telestion-services module and removed ASAP labels Jul 21, 2021
fussel178 added a commit to wuespace/telestion-extension-mongodb that referenced this pull request Jul 31, 2021
@fussel178
Copy link
Member

Closing, because code now resides in https://github.com/wuespace/telestion-extension-mongodb.

@fussel178 fussel178 closed this Jul 31, 2021
@fussel178 fussel178 deleted the feat-database-new branch August 1, 2021 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💣 breaking Issue or PR that includes breaking changes 👏 help wanted Extra attention is needed 📄 java Pull requests that update Java code 🎩 telestion-services Everything related to the telestion-services module 🌷 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants