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.

Sign up for GitHub

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

Jump to bottom

[mongodb] Fix quantity type storage, introduce new types #15617

Closed
wants to merge 6 commits into from
Closed

[mongodb] Fix quantity type storage, introduce new types #15617

wants to merge 6 commits into from

Conversation

konradzawadka
Copy link

@konradzawadka konradzawadka commented Sep 18, 2023 •
edited
Loading

To reproduce the issue configure item as here:

Number:Temperature      supplyTemperature                                                             <temperature>    (gTemperature, gAir)       ["Point","Temperature"]    {channel="modbus:data:localhostTCP:temperature:supplyTemperature:number"}

Then value stored in mongodb is:
7.3 °C

Should be:
7.3

Following fix improves storage and also refactor other conversion based on InfluxDB implementation.

@konradzawadka konradzawadka requested a review from a team as a code owner September 18, 2023 13:10
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/issue-with-storage-quanity-date-with-units-in-mongodb/149714/1

@konradzawadka konradzawadka reopened this Sep 18, 2023
@jlaur jlaur changed the title [mongodb]: fix quantity type storage, introduce new types [mongodb] Fix quantity type storage, introduce new types Sep 18, 2023
@konradzawadka
Copy link
Author

@jlaur do you see chance for fix, is the fix valid?

jlaur
jlaur requested changes Oct 7, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, and welcome as a contributor. 🙂

I have provided some initial feedback. Unfortunately I don't know anything about MongoDB, and I'm a bit hesitant to introduce major changes in persistence services, especially those I'm not familiar with.

In the linked community thread you mention that "7.3 °C" is stored rather than "7.3". Can you show the schema for a Number:Dimension item, and also explain why you would rather store it without unit? If "7.3 °C" is stored, what will happen when unit is changed to °F to the item and the values are queried? Will values (or could it) be converted into the new configured unit, or what will actually happen?

How are migrations and backwards compatibility handled with this PR, i.e. users already having stored "7.3 °C" and now going forward storing values without unit?

...ence.mongodb/src/main/java/org/openhab/persistence/mongodb/internal/MongoDBConvertUtils.java

import javax.measure.Unit;

import org.apache.commons.lang3.StringUtils;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid this import. See #7722 for details.

...ence.mongodb/src/main/java/org/openhab/persistence/mongodb/internal/MongoDBConvertUtils.java
String valueStr = obj.getString(FIELD_VALUE);
if (StringUtils.isNumeric(valueStr)) {
Instant i = Instant.ofEpochMilli(new BigDecimal(valueStr).longValue());
ZonedDateTime z = ZonedDateTime.ofInstant(i, TimeZone.getDefault().toZoneId());
Copy link
Contributor

Choose a reason for hiding this comment

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

TimeZoneProvider should be used to get the time-zone configured in openHAB. You can add a ZoneId parameter and let the service provide this.

See for example:

openhab-addons/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcPersistenceService.java

Lines 74 to 79 in f8bcfb0

@Activate
public JdbcPersistenceService(final @Reference ItemRegistry itemRegistry,
final @Reference TimeZoneProvider timeZoneProvider) {
super(timeZoneProvider);
this.itemRegistry = itemRegistry;
}

...ence.mongodb/src/main/java/org/openhab/persistence/mongodb/internal/MongoDBConvertUtils.java
return new DateTimeType(z);
} else {
return new DateTimeType(
ZonedDateTime.ofInstant(obj.getDate(FIELD_VALUE).toInstant(), ZoneId.systemDefault()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. I don't understand why you have chosen a different approach here compared to line 142?

@jlaur jlaur added the awaiting feedback Awaiting feedback from the pull request author label Oct 23, 2023
konradzawadka and others added 5 commits October 27, 2023 14:26
@konradzawadka @jlaur
Update bundles/org.openhab.persistence.mongodb/src/test/java/org/open…
a07ebc1 
…hab/persistence/mongodb/internal/MongoDBConvertUtilsTest.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: konradzawadka <[email protected]>
@konradzawadka @jlaur
Update bundles/org.openhab.persistence.mongodb/src/test/java/org/open…
2155bda 
…hab/persistence/mongodb/internal/MongoDBConvertUtilsTest.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: konradzawadka <[email protected]>
@konradzawadka @jlaur
Update bundles/org.openhab.persistence.mongodb/src/main/java/org/open…
859ad42 
…hab/persistence/mongodb/internal/MongoDBConvertUtils.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: konradzawadka <[email protected]>
@konradzawadka @jlaur
Update bundles/org.openhab.persistence.mongodb/src/main/java/org/open…
8cbd8ac 
…hab/persistence/mongodb/internal/MongoDBConvertUtils.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: konradzawadka <[email protected]>
@konradzawadka @jlaur
Update bundles/org.openhab.persistence.mongodb/src/main/java/org/open…
7ceda3a 
…hab/persistence/mongodb/internal/MongoDBConvertUtils.java

Co-authored-by: Jacob Laursen <[email protected]>
Signed-off-by: konradzawadka <[email protected]>
@lsiepel
Copy link
Contributor

lsiepel commented May 10, 2024

Gentle ping @konradzawadka can you proceed with this PR, by fixing the conflicts and addressing the comments?

@lsiepel
Copy link
Contributor

lsiepel commented Oct 11, 2024

I guess this PR is related to openhab/openhab-core#1954 Woudl be nice if we can proceed with this. Another ping @konradzawadka do you need anything to proceed?

@lsiepel
Copy link
Contributor

lsiepel commented Oct 26, 2024

Also a ping to @marciseli @boomer41 and @ulbi who contributed to this persistence service in the past. Hopefully they can test, confirm, or assist in any way to get this PR finished.

@ulbi
Copy link
Contributor

ulbi commented Oct 26, 2024

@lsiepel Thanks for the ping - I think I've adressed this along with other challenges in #16333 - I recommend to reject this PR, since it does not fit to the current driver code anymore.

@lsiepel
Copy link
Contributor

lsiepel commented Oct 26, 2024

Mainly superseded by #16333

if there is anything left to fix @konradzawadka feel free to open a new PR or issue

@lsiepel lsiepel closed this Oct 26, 2024
@lsiepel lsiepel removed the awaiting feedback Awaiting feedback from the pull request author label Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@konradzawadka @openhab-bot @lsiepel @ulbi @jlaur