How are deleted scalar fields (e.g. `version`) represented in the "diff" data #152

Open dpancic opened 2 years ago

In GitLab by @stefanprobst on Jan 27, 2022, 11:55

e.g. the following approved item has "NEW" as the value for the version field:

curl "https://sshoc-marketplace-api.acdh-dev.oeaw.ac.at/api/tools-services/DHcf5z"

there is also a suggested version of that item, which has the version field removed:

curl "https://sshoc-marketplace-api.acdh-dev.oeaw.ac.at/api/tools-services/DHcf5z/versions/63912" -H "authorization: ${SSHOC_TOKEN}"

when querying the diff:

curl "https://sshoc-marketplace-api.acdh-dev.oeaw.ac.at/api/tools-services/DHcf5z/diff?with=DHcf5z&otherVersionId=63912" -H "authorization: ${SSHOC_TOKEN}"

the version field is omitted in result.other. it is currently unclear if this signifies that the field has been deleted, or that is is unchanged?

for comparison, this suggested version did not change the version field (the value is still "NEW"), however, here version is also omitted from the diff response:

curl "https://sshoc-marketplace-api.acdh-dev.oeaw.ac.at/api/tools-services/DHcf5z/diff?with=DHcf5z&otherVersionId=63913" -H "authorization: ${SSHOC_TOKEN}"

In GitLab by @stefanprobst on Jan 27, 2022, 12:05

btw. i think this is only an issue for the version field, since label and description are mandatory anyway, and for array fields it's clear.

In GitLab by @tparkola on Jan 31, 2022, 14:16

@stefanprobst Because version field is optional, therefore can be null. All null values are omitted in the response its default application adopted settings. (basic/single values are omitted, but only array (more complex data structures) are send with null as first value.

In GitLab by @stefanprobst on Jan 31, 2022, 14:51

so that means there is no way to judge by the response of the /diff endpoint if a version field is unchanged or if it has been deleted?

e.g. what does this mean:

{
  "item": { "version": "1.0.0", ... },
  "other": { ... }
}

In GitLab by @tparkola on Jan 31, 2022, 14:59

Yes, you are right. In case above you can't tell whether is null or the same.

In GitLab by @stefanprobst on Jan 31, 2022, 16:19

maybe we can use explicit null vs. omitted field for the two states deleted / unchanged?

In GitLab by @tparkola on Feb 1, 2022, 10:33

@stefanprobst Yes, we can establish that omitted string will be read as no changes are made. I think this convention should also apply to sourceItemId and source as well. Even if null is set exlicityly, due to annotation, null fields in response will be omitted, therefore sugessted solution with omitted ot the-same string. Will it be suitable for frontend to make changes regarding this as well?

In GitLab by @stefanprobst on Feb 1, 2022, 12:16

sounds great! :+1: editing source/sourceItemId is (at least currently) not exposed via frontend forms, but applying the convention there as well makes sense.

In GitLab by @tparkola on Feb 1, 2022, 15:36

@stefanprobst The diff was modified, but I changed to 'unaltered', because I thought that this keyword would sound better.

In GitLab by @stefanprobst on Feb 2, 2022, 13:48

@tparkola thanks! quick question: does this apply to the thumbnail field as well?

In GitLab by @tparkola on Feb 2, 2022, 15:13

No, but it can be done as with source, the object will remain the same, if no changes will be made. So it wasn't mistaken with null value.

In GitLab by @stefanprobst on Feb 2, 2022, 17:22

ok, so to make sure, are the following assumptions for non-array correct?:

non-array field	unchanged	deleted
label	omitted	n/a
description	omitted	n/a
version	'unaltered'	omitted
dateCreated	'unaltered'	omitted
dateLastModified	'unaltered'	omitted
sourceItemId	'unaltered'	omitted
source	omitted	null
thumbnail	omitted	null

In GitLab by @tparkola on Feb 3, 2022, 12:25

non-array field	unchanged	unchanged
label	omitted	n/a
description	omitted	n/a
version	'unaltered'	omitted
dateCreated	not compared	not compared
lastInfoUpdate	not compared	not compared
sourceItemId	'unaltered'	omitted
source	omitted	null (but in response omitted due to annotation)
thumbnail	omitted	null (but in response omitted due to annotation)

The main problem is with source and thumbnail. I am thinking about solution how to differentiate them.

In GitLab by @stefanprobst on Feb 3, 2022, 12:50

about dateCreated and dateLastModified (different from lastInfoUpdate) - these are additional fields of Dataset, Publication and TrainingMaterial, but they are not present on Tool and Workflow. the fields can be edited via frontend, so i think it would be good to include them in the diffing.

In GitLab by @tparkola on Feb 11, 2022, 12:37

I looked into dates, its the same case as with String value, therefor I propose variable ZoneDateTime: 0000-01-01T00:00Z [UTC] as for the signal that the date (dateCreated or dateLastModified) didn't change.

In GitLab by @laureD19 on Jun 22, 2022, 11:27

I made a test on dev:

submit a few changes as contributor (version, date created and thumbnail)
as moderator, I can see the diff in review mode for version and thumbnail, but not for date created - see here: https://sshoc-marketplace.acdh-dev.oeaw.ac.at/training-material/ckDYaV/version/67436/review

Would it be possible to include dates in the diff as well, or is it too complicated?

In GitLab by @laureD19 on Jul 22, 2022, 11:45

same test on stage. issue still not solved.

Looking into this issue it seems to me, that backend already solved it (using the API endpoint for checking it):

if dateCreated/dateLastUpdated has changed, you can see the new value in the diff if dateCreated/dateLastUpdated was deleted, it does not appear in the diff

So for this cases, @stefanprobst can you have a look if it is not implemented in frontend?

Besides, I had a problem when I tried to approve an item where only dateCreated/dateLastUpdated was deleted. I get a 500 with the error message:

ERROR e.s.m.c.MarketplaceExceptionHandler.handleNotModifiedException - Exception
eu.sshopencloud.marketplace.services.items.exception.VersionNotChangedException: Provided version has not changed.
at eu.sshopencloud.marketplace.services.items.ItemCrudService.updateItem(ItemCrudService.java:201)
at eu.sshopencloud.marketplace.services.items.TrainingMaterialService.updateTrainingMaterial(TrainingMaterialService.java:84)
at eu.sshopencloud.marketplace.services.items.TrainingMaterialService$$FastClassBySpringCGLIB$$1b740fe6.invoke(<generated>)

@tparkola That would be something to look into (I had no problem if the dates were changed, then approve worked but in the case of deleting them this error occurs).

@KlausIllmayer could you please confirm the issue you mentioned in the backend still exists (500 error). I could not replicate this bug. If you could provide step by step description - that would be very much appreciated. Thanks.

Same, I am not able to run the diff request at all even when authenticated.

OK, could you please provide step by step instruction on how to get the error? and what is the request for the diff you are trying to execute? I need some more details to verify that. As mentioned before I did checks based on what Klaus described, but could not find any problems.

SSHOC / sshoc-marketplace-backend

How are deleted scalar fields (e.g. `version`) represented in the "diff" data #152