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(api-v2): Accept custom timestamps in update/delete requests #1686

Merged
merged 11 commits into from Aug 14, 2020

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Aug 11, 2020

https://dasch.myjetbrains.com/youtrack/issue/DSP-399

  • Accept a custom timestamp when:
    • updating a value
    • marking a value as deleted
    • marking a resource as deleted
  • Add docs.
  • Add tests:
    • updating a value
    • marking a value as deleted
    • marking a resource as deleted

This also fixes a bug: the custom timestamp submitted when creating a value should be knora-api:valueCreationDate, not knora-api:creationDate (still works, but now deprecated in this context). TODO: mention this in the next release notes.

@benjamingeer benjamingeer marked this pull request as draft Aug 11, 2020
@benjamingeer benjamingeer self-assigned this Aug 11, 2020
@benjamingeer benjamingeer added the API/V2 label Aug 11, 2020
…eleted.

- Add docs.
@benjamingeer benjamingeer marked this pull request as ready for review Aug 12, 2020
@benjamingeer benjamingeer requested a review from SepidehAlassi Aug 12, 2020
@SepidehAlassi
Copy link
Contributor

@SepidehAlassi SepidehAlassi commented Aug 13, 2020

@benjamingeer I will review it this afternoon.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Aug 13, 2020

Thanks!

Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

Can you please add test data for both update and delete value with a custom date? Apart from that looks good, I just had a few remarks about comments.

value, explaining why it has been marked as deleted

The optional property `knora-api:deleteDate` (an [xsd:dateTimeStamp](https://www.w3.org/TR/xmlschema11-2/#dateTimeStamp))
specifies a custom timestamp indicating when the value was deleted. If not specified, the current time is used.å

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Aug 13, 2020
Contributor

what does the Swedish character å do at the end of this line? :-D I actually like it giving knora a Scandinavian flavor. Did you know this single character means "a river"?

This comment has been minimized.

@benjamingeer

benjamingeer Aug 14, 2020
Author Collaborator

I often find those "rivers" in my code, I think it happens when I accidentally press Option-A instead of Command-S. 😀

@@ -853,6 +852,11 @@ class ValuesResponderV2(responderData: ResponderData) extends Responder(responde
_ = if (currentValue.valueContent.valueType != submittedExternalValueType.toOntologySchema(InternalSchema)) {
throw BadRequestException(s"Value <$valueIri> has type <${currentValue.valueContent.valueType.toOntologySchema(ApiV2Complex)}>, but the submitted type was <$submittedExternalValueType>")
}

// If a custom value creation date was submitted, make sure it's later than the date of the current version.

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Aug 13, 2020
Contributor

this check did not come to my mind back then when I worked on custom creation date. I trust clients too much I guess that I had not thought they might give a date in the past!

This comment has been minimized.

@benjamingeer

benjamingeer Aug 14, 2020
Author Collaborator

I thought that if the user did make that mistake, it would totally mess up the version history of the value, because the version history query depends on the timestamps being in the right order.

- Fix typo.
- Add e2e tests.
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Aug 14, 2020

Does this look OK now?

Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

It is great, thank you!

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Aug 14, 2020

Thanks for the review, see you in a week!

@subotic
Copy link
Collaborator

@subotic subotic commented Aug 14, 2020

Maybe a few remarks, regarding breaking changes.

We are allowed to make braking changes to features, provided we have a sufficiently long deprecation timeframe. We talked about a timeframe of 12 months so that external projects using the backend API have sufficient time to react before their application will potentially break. The deprecation timer starts running when we make an official release and deploy it into production. This also requires that the release notes mention the deprecation prominently.

If we see that no one is using a deprecated feature anymore, which will require to log usage of deprecated features, then we can remove it sooner, or even immediately.

In the case of the mentioned deprecation in this PR, the deprecation timer will start as soon as we make a non-RC release of v13 and deploy it into production.

@benjamingeer benjamingeer merged commit 0fbe5a8 into develop Aug 14, 2020
7 checks passed
7 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
Publish (on release only)
Details
@benjamingeer benjamingeer deleted the wip/DSP-399-timestamp branch Aug 14, 2020
@subotic subotic added the enhancement label Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants