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

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

Open
dpancic opened this issue Jan 27, 2022 · 20 comments
Open
Assignees
Labels

Comments

@dpancic
Copy link
Contributor

dpancic commented Jan 27, 2022

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}"
@dpancic
Copy link
Contributor Author

dpancic commented Jan 27, 2022

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.

@dpancic
Copy link
Contributor Author

dpancic commented Jan 31, 2022

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.

@dpancic
Copy link
Contributor Author

dpancic commented Jan 31, 2022

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": { ... }
}

@dpancic
Copy link
Contributor Author

dpancic commented Jan 31, 2022

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.

@dpancic
Copy link
Contributor Author

dpancic commented Jan 31, 2022

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?

@dpancic
Copy link
Contributor Author

dpancic commented Feb 1, 2022

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?

@dpancic
Copy link
Contributor Author

dpancic commented Feb 1, 2022

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

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

@dpancic
Copy link
Contributor Author

dpancic commented Feb 1, 2022

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.

@dpancic
Copy link
Contributor Author

dpancic commented Feb 2, 2022

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

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

@dpancic
Copy link
Contributor Author

dpancic commented Feb 2, 2022

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.

@dpancic
Copy link
Contributor Author

dpancic commented Feb 2, 2022

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

@dpancic
Copy link
Contributor Author

dpancic commented Feb 3, 2022

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.

@dpancic
Copy link
Contributor Author

dpancic commented Feb 3, 2022

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.

@dpancic
Copy link
Contributor Author

dpancic commented Feb 11, 2022

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.

@dpancic
Copy link
Contributor Author

dpancic commented Jun 22, 2022

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

I made a test on dev:

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

@dpancic
Copy link
Contributor Author

dpancic commented Jul 22, 2022

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

same test on stage.
issue still not solved.

@vronk vronk added this to the Q1 2023 milestone Jan 16, 2023
@KlausIllmayer
Copy link
Contributor

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).

@tparkola
Copy link
Contributor

@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.

@mkrzmr mkrzmr removed this from the Q2 2023 milestone Mar 7, 2024
@laureD19 laureD19 added this to the Q2 2024 milestone Apr 11, 2024
@mkrzmr
Copy link
Contributor

mkrzmr commented Apr 12, 2024

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

@tparkola
Copy link
Contributor

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.

@laureD19 laureD19 removed this from the Q2 2024 milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants