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

Permanent saving of some types #471

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

Conversation

granit1986
Copy link

Several types such as DateTime, byte[] which are different from redis types, were always detected as modified and saved

@granit1986
Copy link
Author

@slorello89 I don`t understand what it's happened. Local this tests works fine

@slorello89
Copy link
Member

slorello89 commented Jul 30, 2024

@granit1986 - it looks like you are encountering an issue with timezones, I don't think there's anything wrong with thow the code is operating at least insofar as DateTime is concerned. A DateTime is converted to a DateTimeOffset with the local timezone information so it can be converted back and forth to the same DateTime as the server that constructed it was using.

I'm able to revert your changes and still have those tests pass.

@granit1986
Copy link
Author

@slorello89

Great. Thank you. I think we can leave it like this.

@slorello89
Copy link
Member

@granit1986 - my point was that given I'm able to revert your code changes and still have your tests pass, I don't know that this PR is necessary?

@granit1986
Copy link
Author

granit1986 commented Jul 31, 2024

I was hoping that these fixes would make it into the main repository.

@slorello89
Copy link
Member

@granit1986 - I understand, but your tests pass against the original code base, so it's unclear what's being fixed by them, it sounds more like you were having a timezone issue with your DateTimes then an actual defect in Redis OM.

@granit1986
Copy link
Author

I understand. I will try fix test for new case

collection.Update(item);

var timestamp = new DateTimeOffset(dateTime).ToUnixTimeMilliseconds();
_substitute.Received(0).Execute(
Copy link
Author

Choose a reason for hiding this comment

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

@slorello89 i changed test.
here it is implied that the date update should not occur if the value has not changed

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.

2 participants