Speed up annotation list

[frcroth]

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• test the annotations list in the dashboard
• view an annotation
• download an annotation from the list
• some monkey-testing

TODOs:

• adapt frontend @philippotto

Issues:

• fixes Speed up Annotation List Request #6779

---

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

• Updated changelog
• Considered common edge cases

[philippotto]

@frcroth In the front-end, we already distinguished between the types APIAnnotation (e.g., returned by /api/annotations/${annotationId}/info) and APIAnnotationCompact (e.g., returned by /api/users/${userId}/annotations).

How is this distinguished/named in the back-end? Should I handle it like this in the front-end?

• keep APIAnnotation
• rename APIAnnotationCompact to APIAnnotationInfo
• add APIAnnotationInfoCompact

Or am I getting this wrong?

[frcroth]

Yes, it seems there are now 3 degrees of compactness, full, somewhat compact from list and compact from list query with compact query set.

* keep `APIAnnotation`

* rename `APIAnnotationCompact` to `APIAnnotationInfo`

* add `APIAnnotationInfoCompact`

This seems sensible to me.

[philippotto]

@frcroth I noticed that the owner and team properties are flattened into the new compact type like this:

ownerId: ObjectId,
ownerFirstName: String,
ownerLastName: String,
teamId: ObjectId,
teamName: String,
teamOrganizationId: ObjectId,

Could you nest them like this so that it matches the format of the existing APIAnnotationInfo?

owner: {
  id: ObjectId,
  firstName: String,
  lastName: String,
},
team: {
  id: ObjectId,
  name: String,
  organizationId: ObjectId,
}

Also I'm wondering why the team property is not an array (and then teams)? I think, an annotation can have multiple teams assigned? At least, the existing APIAnnotationInfo type is defined like this.

[philippotto]

Also I noticed that the route GET /annotations/shared controllers.AnnotationController.sharedAnnotations() is not used by the front-end anymore. Maybe it's a good idea to remove the back-end related code for that.

[philippotto]

Also, statistics should be named stats (as it's done in APIAnnotationInfo).

[philippotto]

And I'm afraid, we need the tags property, as well. The good news is that formattedHash seems to be unused in the front-end. So, that could be removed altogether.

[philippotto]

Sorry for the peu á peu messaging. state is also required. I hope this is the last missing piece.

Also, when downloading an annotation, we currently check whether the annotation has a volume layer. This information could be found out via an additional request, though. So, no need to change anything for now.

[frcroth]

@philippotto I renamed stats and added state, tags and the correct teams. Is the non-compact list still useful? otherwise I could also remove that code path.

[philippotto]

@philippotto I renamed stats and added state, tags and the correct teams.

Great 👍 I integrated it and the dashboard works now.

Is the non-compact list still useful? otherwise I could also remove that code path.

Yes, it can be removed and I think we should do this to reduce complexity. These routes should then return the compact annotation version, too:

• /api/user/annotations
• /api/users/${userId}/annotations

After removal, the compact GET parameter shouldn't exist, anymore.

[philippotto]

Yes, it can be removed and I think we should do this to reduce complexity.

We need to check compatibility with wklibs, too. @fm3 can give insights probably, since he just reworked the client in wklibs.

[fm3]

Good point! That’s true, this would be a breaking change for the libs. scalableminds/webknossos-libs#948 makes the libs use versioned api routes. This means that we can support the old case by supplying the older version in the LegacyApiController. However, many users might not yet have adopted to that new libs version. Let’s wait some time before introducing this kind of breaking change. I’d say in this PR it would be best to have this be opt-in only.

[frcroth]

Good point! That’s true, this would be a breaking change for the libs. scalableminds/webknossos-libs#948 makes the libs use versioned api routes. This means that we can support the old case by supplying the older version in the LegacyApiController. However, many users might not yet have adopted to that new libs version. Let’s wait some time before introducing this kind of breaking change. I’d say in this PR it would be best to have this be opt-in only.

I think it wouldn't be difficult to adapt this so that everything that was returned in the previous non-compact way is now also part of the compact path (and still use only one query). In that case, the old way could be removed, couldn't it?
I think the only relevant things missing are annotation layers and org.

[fm3]

sounds fair, yes

[philippotto]

Ok, then, I'd suggest to do the comparison between single-sql-statement-with-layers-and-org and single-sql-statement-without-layers-and-org to decide whether it's okay to always include layers and organization.

Just tested it, no measurable difference for my test instance (of course I only have one organization etc).

Ok, great 👍 Then, let's keep it simple and just go with one type. I cleaned up the front-end and undid the unnecessary changes. I took the opportunity and renamed the APIAnnotationCompact type to APIAnnotationInfo to match the back-end's nomenclature better. Let me know in case you need something else from the front-end.

[frcroth]

Ok, then, I'd suggest to do the comparison between single-sql-statement-with-layers-and-org and single-sql-statement-without-layers-and-org to decide whether it's okay to always include layers and organization.

Just tested it, no measurable difference for my test instance (of course I only have one organization etc).

Ok, great 👍 Then, let's keep it simple and just go with one type. I cleaned up the front-end and undid the unnecessary changes. I took the opportunity and renamed the APIAnnotationCompact type to APIAnnotationInfo to match the back-end's nomenclature better. Let me know in case you need something else from the front-end.

Nice! I think you could assign someone for frontend review then if you believe the changes are non-trivial.

[fm3]

Nice 😎 Please see my small comments

[MichaelBuessemeyer]

Frontend LGTM and I did not find any bugs during testing with the dev instance.

[fm3]

Looks like the formattedHash field is missing, which is why the snapshot tests fail. Is this field used somewhere? Should we just refresh the snapshots? The libs client does not use it

[fm3]

@MichaelBuessemeyer Note that since the CI wasn’t green recently, maybe the dev instance you tested on was outdated

[MichaelBuessemeyer]

Looks like the formattedHash field is missing, which is why the snapshot tests fail. Is this field used somewhere? Should we just refresh the snapshots? The libs client does not use it

Yes it seems like it is not needed anymore: #7410 (comment)

And I'm afraid, we need the tags property, as well. The good news is that formattedHash seems to be unused in the front-end. So, that could be removed altogether.

@MichaelBuessemeyer Note that since the CI wasn’t green recently, maybe the dev instance you tested on was outdated

Oh, thanks for the notice. I should do a quick retest once the CI is green again 👍

[fm3]

@MichaelBuessemeyer I just installed the new version as a dev instance :)

[MichaelBuessemeyer]

I just installed the new version as a dev instance :)

Thanks 🙏.
I did not find any issues 🚀