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

DM API: Add timestamps in TransferProcess DTO #1350

Merged
merged 4 commits into from
May 30, 2022
Merged

DM API: Add timestamps in TransferProcess DTO #1350

merged 4 commits into from
May 30, 2022

Conversation

algattik
Copy link
Contributor

What this PR changes/adds

Add timestamps when querying TransferProcesses in Data management API.

Why it does that

Useful for web front-ends, to allow to sort transfers by dates.

Further notes

Linked Issue(s)

Closes #1297

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

algattik and others added 3 commits May 24, 2022 13:56
@algattik
Feature/1297/264 TP DTO timestamps (#265)
7ba5590
@algattik
Squashed commit of the following:
8344f85 
commit 86325b3
Author: Alexandre Gattiker <[email protected]>
Date:   Mon May 23 22:45:53 2022 +0200

    test

commit a745a1b
Author: Alexandre Gattiker <[email protected]>
Date:   Mon May 23 08:37:02 2022 +0200

    .

commit eefdca9
Author: Alexandre Gattiker <[email protected]>
Date:   Mon May 23 06:54:03 2022 +0200

    Update DecentralizedIdentityServiceExtension.java

commit e72ccae
Author: Alexandre Gattiker <[email protected]>
Date:   Mon May 23 06:53:58 2022 +0200

    Update IdentityDidCoreExtension.java

commit 7f7641f
Author: Alexandre Gattiker <[email protected]>
Date:   Mon May 23 05:42:52 2022 +0200

    .

commit bd03436
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 22:40:22 2022 +0200

    .

commit c8cce8d
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:28:14 2022 +0200

    lib -> ext

commit 4006ba6
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:07:58 2022 +0200

    Update IdentityService.java

commit 33ac838
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:07:19 2022 +0200

    Update IdsValidationRuleTest.java

commit aaff65b
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:06:54 2022 +0200

    Update IdsTokenValidationServiceExtension.java

commit 4e0105a
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:06:26 2022 +0200

    Update IdsResponseMessageFactoryTest.java

commit 22f822c
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:03:11 2022 +0200

    Update IdsMultipartSenderTest.java

commit f0be333
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:02:36 2022 +0200

    Update AbstractMultipartDispatcherIntegrationTest.java

commit 07ab718
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:02:16 2022 +0200

    Update IdsMultipartSender.java

commit 8f79a50
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:02:03 2022 +0200

    .

commit 50f1a91
Author: Alexandre Gattiker <[email protected]>
Date:   Sun May 22 09:00:44 2022 +0200

    .

commit f76a99f
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 08:33:28 2022 +0200

    .

commit 2407b2a
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 08:04:28 2022 +0200

    hook

commit 6f42eb4
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 07:51:22 2022 +0200

    .

commit 8e5e6cd
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 07:37:39 2022 +0200

    i

commit b64b6cc
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 07:26:01 2022 +0200

    .

commit b943b0a
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 07:06:55 2022 +0200

    .

commit adabc16
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 06:47:48 2022 +0200

    r

commit 8b34811
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 06:40:58 2022 +0200

    i

commit be66376
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 06:30:06 2022 +0200

    .

commit 23a5bb5
Author: Alexandre Gattiker <[email protected]>
Date:   Sat May 21 06:06:36 2022 +0200

    i

commit 1b51386
Author: Alexandre Gattiker <[email protected]>
Date:   Fri May 20 13:50:30 2022 +0200

    Update DecentralizedIdentityServiceTest.java

commit 2f121c6
Author: Alexandre Gattiker <[email protected]>
Date:   Fri May 20 13:48:25 2022 +0200

    .
Merge branch 'feature/1297-tp-dto-timestamps' of https://github.com/agera-edc/DataSpaceConnector into feature/1297-tp-dto-timestamps
@algattik
.
246eaa6
@codecov-commenter
Copy link

Codecov Report

Merging #1350 (246eaa6) into main (ead9b4d) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1350      +/-   ##
==========================================
+ Coverage   67.59%   67.64%   +0.04%     
==========================================
  Files         719      719              
  Lines       15878    15900      +22     
  Branches     1045     1045              
==========================================
+ Hits        10733    10755      +22     
  Misses       4670     4670              
  Partials      475      475
Impacted Files Coverage Δ
...sfer/core/transfer/TransferProcessManagerImpl.java 79.70% <100.00%> (+0.37%) ⬆️
...ment/transferprocess/model/TransferProcessDto.java 100.00% <100.00%> (ø)
...ransferProcessToTransferProcessDtoTransformer.java 91.30% <100.00%> (+0.82%) ⬆️
.../sql/transferprocess/store/PostgresStatements.java 100.00% <100.00%> (ø)
...transferprocess/store/SqlTransferProcessStore.java 84.93% <100.00%> (+0.18%) ⬆️
...rprocess/store/TransferProcessStoreStatements.java 100.00% <100.00%> (ø)
...tor/spi/types/domain/transfer/TransferProcess.java 76.33% <100.00%> (+0.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ead9b4d...246eaa6. Read the comment docs.

@algattik algattik changed the title Add timestamps in TransferProcess DTO DM API: Add timestamps in TransferProcess DTO May 24, 2022
@algattik algattik marked this pull request as ready for review May 24, 2022 19:17
@eclipse-edc-bot
Copy link
Contributor

Can one of the admins verify this patch?

paullatzelsperger
paullatzelsperger reviewed May 25, 2022
View reviewed changes
...n/java/org/eclipse/dataspaceconnector/transfer/core/transfer/TransferProcessManagerImpl.java
@@ -52,6 +52,7 @@
import org.eclipse.dataspaceconnector.spi.types.domain.transfer.TransferProcessStates;
import org.eclipse.dataspaceconnector.spi.types.domain.transfer.command.TransferProcessCommand;

import java.time.Clock;
Copy link
Member

Choose a reason for hiding this comment

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

any reason why Instant.now().epochMilli() is not acceptable? That would be consistent with the rest of the code base, and it would avoid having to instantiate a member variable.

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, this is used in test and is standard best practice. Any reason not to?

One improvement down the line could be to make the clock global.

Copy link
Member

@paullatzelsperger paullatzelsperger May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

if we decide to use a "global" clock instead of Instant.now().... we should do it in one fell swoop across the code base in a dedicated PR, not piece-by-piece.

.../src/main/java/org/eclipse/dataspaceconnector/spi/types/domain/transfer/TransferProcess.java
@@ -419,6 +425,11 @@ public Builder type(Type type) {
return this;
}

public Builder createdTimestamp(long value) {
process.createdTimestamp = value;
Copy link
Member

Choose a reason for hiding this comment

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

maybe the build() method could initialize the createdTimestamp = Instant.now().epochMilli() if it is null? Alternatively this could also be done in the private CTor of TransferProcess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For defensive reasons I think it's better to do it this way to avoid side effects. For example, if the row is null in a SQL backend, the behavior you suggest would erroneously populate it with the current date.

ndr-brt
ndr-brt approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

Great, I would suggest additional issues to add this timestamp also to other entities and to standardize the use of Clock on all the codebase.

@algattik algattik mentioned this pull request May 30, 2022
Closed
paullatzelsperger
paullatzelsperger approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

agreed to homogenize the use of Clock across the code base in #1364

@paullatzelsperger paullatzelsperger merged commit 348ca79 into eclipse-edc:main May 30, 2022
@algattik algattik mentioned this pull request May 30, 2022
Closed
@algattik algattik deleted the feature/1297-tp-dto-timestamps branch June 22, 2022 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndr-brt ndr-brt ndr-brt approved these changes

@paullatzelsperger paullatzelsperger paullatzelsperger approved these changes

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

Successfully merging this pull request may close these issues.

Include TransferProcess timestamps in Data Management API
5 participants
@algattik @codecov-commenter @eclipse-edc-bot @ndr-brt @paullatzelsperger