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

Support $currentDate field update operator #662

Merged
merged 51 commits into from
Jun 7, 2022
Merged

Support $currentDate field update operator #662

merged 51 commits into from
Jun 7, 2022

Conversation

seeforschauer
Copy link
Contributor

@seeforschauer seeforschauer commented May 31, 2022

Closes #622.

@seeforschauer seeforschauer self-assigned this May 31, 2022
@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #662 (6ec104e) into main (88af5f4) will increase coverage by 0.36%.
The diff coverage is 82.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #662      +/-   ##
==========================================
+ Coverage   63.85%   64.21%   +0.36%     
==========================================
  Files         192      193       +1     
  Lines        7694     7814     +120     
==========================================
+ Hits         4913     5018     +105     
- Misses       2225     2233       +8     
- Partials      556      563       +7     
Impacted Files Coverage Δ
internal/types/types.go 85.88% <ø> (ø)
internal/handlers/common/update.go 87.50% <79.79%> (-5.12%) ⬇️
internal/util/testutil/path.go 79.68% <80.00%> (+16.52%) ⬆️
integration/helpers.go 77.77% <100.00%> (ø)
internal/types/timestamp.go 100.00% <100.00%> (ø)
internal/clientconn/listener.go 73.86% <0.00%> (-3.41%) ⬇️
internal/wire/msg_header.go 69.04% <0.00%> (-2.39%) ⬇️
internal/wire/msg_body.go 36.50% <0.00%> (-1.59%) ⬇️
internal/types/document.go 82.63% <0.00%> (+1.19%) ⬆️
Flag Coverage Δ
FerretDB 60.37% <82.25%> (+0.57%) ⬆️
MongoDB 7.46% <13.70%> (+1.13%) ⬆️
integration 60.40% <82.25%> (+0.57%) ⬆️
unit 23.02% <0.00%> (-0.38%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@seeforschauer seeforschauer marked this pull request as ready for review June 1, 2022 14:31
@seeforschauer
Copy link
Contributor Author

seeforschauer commented Jun 1, 2022

I invite you not to review but to discuss.
I need somehow to test data time values.
And all my ideas are overcomplicated.
Can we do it a way that is more simple?
Do we have an agreed way on how to test it?

@AlekSi AlekSi added the code/feature Some user-visible feature is not implemented yet label Jun 1, 2022
@AlekSi AlekSi added this to the v0.3.1 milestone Jun 1, 2022
@seeforschauer seeforschauer marked this pull request as draft June 2, 2022 06:19
internal/handlers/common/update.go Outdated Show resolved Hide resolved
internal/handlers/common/update.go Outdated Show resolved Hide resolved
w84thesun
w84thesun previously approved these changes Jun 6, 2022
w84thesun
w84thesun previously approved these changes Jun 6, 2022
@seeforschauer
Copy link
Contributor Author

seeforschauer commented Jun 6, 2022

@w84thesun @rumyantseva PTAL, I've changed to time.Time

Copy link
Member

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

LGTM!

@seeforschauer seeforschauer merged commit fb8b9d6 into FerretDB:main Jun 7, 2022
@seeforschauer seeforschauer deleted the issue-622-currentDate branch June 7, 2022 09:15
@AlekSi
Copy link
Member

AlekSi commented Jun 7, 2022

Autogenerated GitHub's squash commit's title and body should be manually replaced by PR's title and description; that includes both manual merges and auto-merges.

fb8b9d6 commit body is empty

Comment on lines +915 to +921
testutil.CompareAndSetByPathTime(
t,
expected,
actual,
maxDifference,
path,
)
Copy link
Member

Choose a reason for hiding this comment

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

Those arguments are short enough to be put on the same line

Comment on lines +922 to +923
expected.RemoveByPath(path)
actual.RemoveByPath(path)
Copy link
Member

Choose a reason for hiding this comment

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

We do we set values by path only to remove them on the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do we set values by path only to remove them on the next line?

There is assert.Equal(t, expected, actual) below, and it tests whether other document values are ok (not changed, not removed, not updated anyhow).

t.Parallel()

// maxDifference is a maximum amount of seconds can differ the value in placeholder from actual value
maxDifference := time.Duration(60 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

That values should be that large. We are in a serious trouble if that test runs for 60 seconds

Comment on lines +728 to +729
now := primitive.NewDateTimeFromTime(time.Now().UTC())
nowTimestamp := primitive.Timestamp{T: uint32(time.Now().UTC().Unix()), I: uint32(0)}
Copy link
Member

Choose a reason for hiding this comment

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

.UTC() does nothing there

assert.WithinDuration(tb, expectedT, actualT, delta)

default:
tb.FailNow()
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a better error message there

}

// DateTime returns time.Time ignoring increment.
func DateTime(t Timestamp) time.Time {
Copy link
Member

Choose a reason for hiding this comment

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

Let's make it a method: func (t Timestamp) Time() time.Time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's make it a method: func (t Timestamp) Time() time.Time

beautiful ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's make it a method: func (t Timestamp) Time() time.Time

changed.

// DateTime returns time.Time ignoring increment.
func DateTime(t Timestamp) time.Time {
t >>= 32
return time.UnixMilli(int64(t)).UTC()
Copy link
Member

Choose a reason for hiding this comment

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

.UTC() does nothing there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.UTC() does nothing there

changed.

func NextTimestamp(t time.Time) Timestamp {
sec := t.UTC().Unix()
c := atomic.AddUint32(&timestampCounter, 1)
sec <<= 32
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we could just call NewTimestamp(t, c) there to avoid copying the same code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we could just call NewTimestamp(t, c) there to avoid copying the same code

amazing.


// NewTimestamp returns a timestamp from time and an increment.
func NewTimestamp(t time.Time, c uint32) Timestamp {
sec := t.UTC().Unix()
Copy link
Member

Choose a reason for hiding this comment

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

.UTC() does nothing there. Unix timestamps are always in UTC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.UTC() does nothing there. Unix timestamps are always in UTC

changed.

Copy link
Contributor Author

@seeforschauer seeforschauer left a comment

Choose a reason for hiding this comment

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

Thank you for the review, @AlekSi, 👍
I've added changes in #701.

func NextTimestamp(t time.Time) Timestamp {
sec := t.UTC().Unix()
c := atomic.AddUint32(&timestampCounter, 1)
sec <<= 32
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we could just call NewTimestamp(t, c) there to avoid copying the same code

amazing.


// NewTimestamp returns a timestamp from time and an increment.
func NewTimestamp(t time.Time, c uint32) Timestamp {
sec := t.UTC().Unix()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.UTC() does nothing there. Unix timestamps are always in UTC

changed.

}

// DateTime returns time.Time ignoring increment.
func DateTime(t Timestamp) time.Time {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's make it a method: func (t Timestamp) Time() time.Time

beautiful ❤️

// DateTime returns time.Time ignoring increment.
func DateTime(t Timestamp) time.Time {
t >>= 32
return time.UnixMilli(int64(t)).UTC()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.UTC() does nothing there

changed.

}

// DateTime returns time.Time ignoring increment.
func DateTime(t Timestamp) time.Time {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's make it a method: func (t Timestamp) Time() time.Time

changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/feature Some user-visible feature is not implemented yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support $currentDate field update operator
4 participants