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
Conversation
@benjamingeer I will review it this afternoon. |
Thanks! |
There was a problem hiding this 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.å |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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. 😀
.../main/scala/org/knora/webapi/messages/v2/responder/resourcemessages/ResourceMessagesV2.scala
Show resolved
Hide resolved
...pi/src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala
Show resolved
Hide resolved
@@ -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. |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
Does this look OK now? |
There was a problem hiding this 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!
Thanks for the review, see you in a week! |
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 |
https://dasch.myjetbrains.com/youtrack/issue/DSP-399
This also fixes a bug: the custom timestamp submitted when creating a value should be
knora-api:valueCreationDate
, notknora-api:creationDate
(still works, but now deprecated in this context). TODO: mention this in the next release notes.