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

@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 August 11, 2020 10:36
@benjamingeer benjamingeer self-assigned this Aug 11, 2020
@benjamingeer benjamingeer marked this pull request as ready for review August 12, 2020 16:16
@SepidehAlassi
Copy link
Contributor

@benjamingeer I will review it this afternoon.

@benjamingeer
Copy link
Author

Thanks!

Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

Choose a reason for hiding this comment

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

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.å
Copy link
Contributor

Choose a reason for hiding this comment

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

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"?

Copy link
Author

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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
Author

Does this look OK now?

Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

Choose a reason for hiding this comment

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

It is great, thank you!

@benjamingeer
Copy link
Author

Thanks for the review, see you in a week!

@subotic
Copy link
Collaborator

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
@benjamingeer benjamingeer deleted the wip/DSP-399-timestamp branch August 14, 2020 11:53
@subotic subotic added the enhancement improve existing code or new feature label Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API/V2 enhancement improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants