gbv / jskos-server

Web service to access JSKOS data
https://coli-conc.gbv.de/api/
MIT License
6 stars 4 forks source link

Add mapping content identifier to annotations #173

Closed stefandesu closed 2 years ago

stefandesu commented 2 years ago

Let's assume annotations are only used to annotate mappings that live inside the same jskos-server instance.

If the target mapping of an annotation changes in certain ways, i.e. its members or its type is changed, an annotation with motivation assessing (with bodyValue either +1 or -1) should be marked as invalidated or potentially even deleted because it's not relevant anymore.

I personally would not delete an annotation automatically since it belongs to a different user and also the creator of the mapping could simply change a mapping, then change it back, abusing the system. I think the best way would be to simply save a mapping's urn:jskos:mapping:content: and/or urn:jskos:mapping:members: identifiers which can then be compared to the current mapping's identifier to see whether the annotation refers to an old version of that mapping. If we do it this way, then jskos-server doesn't even need to be aware of this mechanism at all. (Except when we implement https://github.com/gbv/cocoda/issues/605 which consists of calculating the total "rating" of a mapping.)

The Web Annotation Data Model is pretty flexible, so I think it would be possible to add additional data to an annotation. Also we could via npm run upgrade add the information to all existing annotations (assuming that the associated mappings have not changed since the annotation was created).

What do you think, @nichtich?

(This came up in https://github.com/gbv/cocoda/issues/667.)

nichtich commented 2 years ago

I would compare annotations to comments and edit suggestions in collaborative software such as Google Docs. If other people comment my text, I can close the comment and the comment disappears when I delete the commented section of text. Trying to prevent "abuse" should not be a technical goal anyway.

On the other hand urn:jskos:mapping:content: is all we need, so adding this identifier to all mappings is probably the way to go.

Additional thought: Annotating a mapping might imply same judgement of other "duplicate" mappings with same urn:jskos:mapping:content:. But that's another issue.

stefandesu commented 2 years ago

The thing is: I would add the urn:jskos:mapping:content: identifier on top of the URI because the URI is the only way we can correctly identify a specific mapping. The question is how we can save this identifier while keeping backwards compatibility and adhering to the Web Annotation Data Model? Maybe we could misuse the canonical field and save it there? It has a somewhat similar meaning. Otherwise we could also just add a custom field like we do with JSKOS all the time. 🙈

nichtich commented 2 years ago

canonical looks good but unforunately it's intended for the canonical URI of the annotation while we need a URI of the mapping. Changing target to urn:jskos:mapping:content: identifier would make sense to me but likely requires several changes in Cocoda as well, no?

stefandesu commented 2 years ago

canonical looks good but unforunately it's intended for the canonical URI of the annotation while we need a URI of the mapping. Changing target to urn:jskos:mapping:content: identifier ~would make sense to me but~ likely requires several changes in Cocoda as well, no?

Yeah, it would require changes in Cocoda, changes in jskos-server, breaks backwards compatibility, and does not make sense in my opinion. I mentioned canonical not because it is the right place for something like this, I know it's not intended to be used for that, that's why I wrote "misuse". Maybe it would be better to just add a custom property...

nichtich commented 2 years ago

Reading the Web Annotation Data Model I found how this feature (making sure the identity of annotated target has not been changed) is intended to be solved:

change

{
  "target": "https://coli-conc.gbv.de/api/mappings/f8eff4e2-a6df-4d2c-8382-523072c59af7",
}

to

{
  "target": {
    "id": "https://coli-conc.gbv.de/api/mappings/981a003d-ae64-46b9-9571-04cbbe78c7fd",
    "state": { 
      "id": "urn:jskos:mapping:content:3df6513d6c645eefafc93e62f6e2f221c6624e1b"
    }
  }
}

but as you said, the change might not be worth the trouble, we could just do

{
  "target": "https://coli-conc.gbv.de/api/mappings/981a003d-ae64-46b9-9571-04cbbe78c7fd",
  "_state": "urn:jskos:mapping:content:3df6513d6c645eefafc93e62f6e2f221c6624e1b"
}
stefandesu commented 2 years ago

Oh, I missed this! Well, I wouldn't mind either option, but actually doing it how the spec is intended sounds right to me. Changing the format of target doesn't break backwards compatibility for Cocoda at least as long as the API requests still work the same way.

One more question: Should Cocoda be responsible for adding that information or should jskos-server do it? I'm actually not sure about this, but so far, jskos-server doesn't actually mess at all with the annotations besides adding a URI in field id and dealing with created and modified.

Edit: It would probably make sense for jskos-server to add that information if the target is part of the same instance. Since it is only additional information, I think it would be fine to add it if it's not already included in the payload.

stefandesu commented 2 years ago

Suggested implementation in branch annotation-state.

stefandesu commented 2 years ago

I found out (unfortunately after almost having finished the implementation) that jskos-server needs to have a consistent format for the target property of annotations. So we can't have both strings and objects (which is what I had previously assumed). There are two solutions:

  1. Make sure all annotations use target.id instead of a string as target, via adjustments to the service methods and an upgrade script that has to be called after an upgrade. This is how it's currently implemented in the branch.
  2. Not use the official model, but rather your last suggestion (maybe STATE instead of _state because jskos-server removes all properties starting with an underscore). This would make things simpler and things wouldn't break if npm run upgrade wasn't called after an upgrade, but our annotation format would not be standard Web Annotation Data Model anymore.

@nichtich what do you think? My guess would be that you'd vote for option 2, but maybe I'm wrong about that. 😅

Edit: Note that both options work fine from a validation standpoint which is nice. target.id is already supported by our JSON Schema, no changes necessary, and additional properties also seem to be allowed.

stefandesu commented 2 years ago

This is now part of v1.4.5 and can be taken into account when implementing https://github.com/gbv/cocoda/issues/605 and https://github.com/gbv/cocoda/issues/667.