WIP: user-defined key-value properties for Segments,Nodes,Trees

[fm3]

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• abc

TODOs:

• [x] Backend
  • [x] Introduce properties in proto definitions
  • [x] Update actions (Create+Update for Segment,Node,Tree)
  • [x] NML Writer
  • [x] NML Parser
  • [x] Unit tests
  • [x] Snapshots
  • [ ] Add assertions on duplicate keys? Might break annotation reloading…
• [ ] Frontend
  • [ ] UI to display/edit properties
  • [ ] Update actions
  • [ ] NML Spec
  • [ ] NML Writer
  • [ ] NML Parser
• [ ] Include this in the end-to-end tests
• [ ] docs
• [ ] follow-up issue for libs

Issues:

• fixes #7483

---

(Please delete unneeded items, merge only when none are left open)

• [ ] Updated changelog
• [ ] Updated migration guide if applicable
• [ ] Updated documentation if applicable
• [ ] Adapted wk-libs python client if relevant API parts change
• [ ] Removed dev-only changes like prints and application.conf edits
• [ ] Considered common edge cases
• [ ] Needs datastore update after deployment

[fm3]

I created a first version of this in the backend. It works for Trees, Nodes and Segments (not yet for groups).

In proto, each of these now has a field repeated UserDefinedPropertyProto userDefinedProperties, a type defined as

message UserDefinedPropertyProto {
  required string key = 1;
  optional string stringValue = 2;
  optional bool boolValue = 3;
  optional double numberValue = 4;
  repeated string stringListValue = 6;
}

They can be set both in the “Create” and “Update” UpdateActions of the respective items. For example, the UpdateNodeSkeletonAction now has an optional field userDefinedProperties: Option[Seq[UserDefinedProperty]] = None (None has the same semantic as empty list). The format for a UserDefinedProperty expected in the json update actions is

key: String,
stringValue: Option[String] = None,
boolValue: Option[Boolean] = None,
numberValue: Option[Double] = None,
stringListValue: Option[Seq[String]] = None

The backend expects all update actions to include the full list for the edited item. So if it was nonempty first and is then updated to None, it is all cleared.

The frontend should ensure that only one of stringValue, boolValue, numberValue, stringListValue is set for each item, and that each key is used only once per item.

I also updated the NML format as written and parsed by the backend to include the userDeinedProperties. Looks like this:

    <userDefinedProperty key="aKey" numberValue="5.7"/>
    <userDefinedProperty key="anotherKey" boolValue="true"/>
    <userDefinedProperty key="thirdKey" stringListValue-0="hello"/>
    <userDefinedProperty key="fourthKey" stringListValue-1="there"/>

[philippotto]

@fm3 I have two questions regarding the nml format:

1. what do you think about having a parent tag for the user defined properties? e.g.:

<thing ...>
  <nodes>...</nodes>
  <edges>...</nodes>
  <userDefinedProperties>
    <userDefinedProperty ... />
    ...
  </userDefinedProperties>
</thing>

I think this would fit more nicely with the existing structure, but I'm happy to be convinced otherwise.

2. Your example for stringListValue uses indices 0 and 1 with different keys. Did you mean it like this?

    <userDefinedProperty key="thirdKey" stringListValue-0="hello" stringListValue-1="there" />

Having all values for the array in the same XML tag would be easier to parse for the front-end because we process each xml tag individually. also it would be less redundant because the key would only be stated once.

what do you think?

[fm3]

a parent tag for the user defined properties

Sure, no objections :)

Did you mean it like this?

Yes!

[philippotto]

@fm3 to keep the naming consistent with the metadata for datasets and folders, we should rename the properties to metadata where possible (single entries in the metadata could still be named properties maybe?). so, userDefinedProperties would probably be userDefinedMetadata.

could you do the renaming in the back-end? I'd do the renaming in the frontend as a followup then.

[fm3]

ok, I’ll have a look

[fm3]

I pushed the renaming (compare slack thread). Note that adapting the backend NML writer+parser to your suggested format with parent tags is still pending. I hope I’ll get to that in the coming days

[philippotto]

@MichaelBuessemeyer I think you can already have a look at the front-end.

[MichaelBuessemeyer]

@fm3 Can I review the backend part?

[fm3]

Yes. There is on open checkbox about which I am unsure (“Add assertions on duplicate keys”). Since the update actions are applied lazily, adding assertions there will break the next annotation loading. On the other hand, maybe there should rather be a readable error message than an inconsistent state. What do you think? I think for the other update actions, the backend does not do much checking :thinking:

Other than that, the backend changes should be complete.

[MichaelBuessemeyer]

What do you think? I think for the other update actions, the backend does not do much checking 🤔

I am unfamiliar with the backend code responsible for receiving the update actions. But in case the code iterates over the actions, one could run a simple duplicate key assertion over the metadata. Of cause, this needs to be done before the update actions are stored to be lazily applied.

Another way to handle this could be to simply choose the first occurring entry with a key when applying the metadata update in the fossildb. Meaning dropping the duplicate key. This is already how the frontend behaves and I would say that this is fair to handle a "broken" update action this way.

On the other hand, maybe there should rather be a readable error message than an inconsistent state.

How do you expect the user to escape from this error message state and recover such a broken annotation?

[fm3]

Another way to handle this could be to simply choose the first occurring entry with a key when applying the metadata update in the fossildb. Meaning dropping the duplicate key. This is already how the frontend behaves and I would say that this is fair to handle a "broken" update action this way.

That’s a good plan, I’ll go for that

[MichaelBuessemeyer]

I just noticed that the border around the empty metadata placeholder in the dashboard is gone:

Maybe change it back to:

The background gray is consistent with the gray used in the metadata table (at least it looks like that)

[fm3]

border around the empty metadata placeholder in the dashboard is gone

@MichaelBuessemeyer good catch, maybe you could fix that here directly?

[MichaelBuessemeyer]

@MichaelBuessemeyer good catch, maybe you could fix that here directly?

Yeah sure, do you think it looks visually fine this way? I mean the second image from my previous post

[fm3]

Yeah sure, do you think it looks visually fine this way? I mean the second image from my previous post

I think it’s fine, yes. Mostly it’s consistent. We can discuss later if we want to adapt the visual design again. (In fact, I’d probably vote to remove the illustration entirely. But not in this PR)