XWIKI-16742 reports that the “User Type” column of the User Directory is empty for almost every user. The reason is that no value is stored: usertype is declared on XWiki.XWikiUsers with Simple as its default value, but a class default value in XWiki is only used to preselect an entry in the edit form. It is never persisted, and it is never used when reading or displaying the property. So a user profile carries a usertype value only once its owner has saved their preferences at least once, which most users never do.
The consequence is not only an empty cell. Sorting and filtering on that column work on the raw stored value, so they do not work as users could expect them to, and no display-side fix can change that.
Problem
Two questions are entangled here, and I would like the community to settle both:
Do we want the usertype value to actually exist in the database for every user?
If yes, do we want a data migration to backfill it on existing wikis?
The second one is where I need your input the most: a migration here means one document save per user missing the property, on every wiki of a farm, at upgrade time. On a large instance that is a lot of saves for a rather small issue.
Solutions
A. Persist at creation + migrate existing users.createUser sets the property to the class-configured default when it is not part of the submitted data, and a data migration does the same for existing user objects that lack it. This is what PR #6104 currently implements.
B. Persist at creation only, no migration. New users now have a value by default. Existing wikis keep the inconsistency forever, or until each user saves their preferences, so the User Directory column filters and sorters stays broken on every instance that exists today.
C. Display-side fix only. No change to user data. Make the list displayer fall back to the class default value in view mode when nothing is stored. This fixes the empty cell everywhere at once and costs no migration, but sorting and filtering stay wrong, and the value shown stops matching the value stored.
D. Make missing values fall back to the class default at read time, for every xproperty. This addresses the actual root cause rather than this one property, but it changes the semantics of every xproperty of every xclass, and “unset” and “set to the default” stop being distinguishable. In some cases this could be a relevant difference.
E. Won’t fix.
Opinion
+1 for A from me, because:
C cannot fix sorting and filtering, which is half of what the issue reports, and it creates a second, subtler inconsistency by displaying a value the database does not hold.
B leaves every current instance broken, which is not ideal IMO.
D is too wide and we’d probably break a lot more things with it.
That said, given the core issue reported in the ticket is so low severity, I’m not sure it’s worth to go with A. Concretely: the migration saves one revision per migrated user document as a minor edit. It is bounded, it runs once, and it only touches objects that are missing the property, but on a farm with many wikis and many users it is for from free. In addition, the base cost of a migration is quite high, they can come with a lot of complications.
Conclusion
Do you agree that the usertype value should be persisted on the object rather than resolved at display time?
Is a data migration backfilling it on existing users worth its cost for this issue, or should we ship the creation-time fix alone and let existing wikis stay as they are?
I’ll close this topic in a week if it doesn’t spur a larger discussion. The current implementation is in PR #6104.
Thank you for your interest in the topic! I’m looking forward to your answers
Lucas C.
Well, not really: the migration could perform batch rows update and doesn’t really need to go in XWikiDocument#save API. Unless we want to avoid discrepancies with history maybe.
I think I’d go for this. Note that maybe we should have a way to display in the LT that we display the default value, so the sorting / filtering would still make sense.
ok that’s a bit too strong probably. I don’t see the real need in this use case but I guess it’s to allow using a default LT results page rather than a custom one.
But it’s indeed always a lot of work to write a migrator (and it impacts our users), so IMO the need would need to be more important to go that way.
Really making sure that you can trust the database to have the default value instead of an empty string (and so not having to care about that in a query, which is always nice) requires much more. For starter the only way in which it makes sense is by changing how we deal with list properties default in general and not just this specific list property. There is also other use cases that need to be fixed: the class migrator probably needs to take into account the use case where a list property which used to not have a default now has one, etc., fix properties which have empty values at save time (because it comes from the history, which is usually not migrated, or from a XAR, for example), etc. Basically the same kind of protections we currently have to make sure all xobjects in the database always contains a property if the class say they should (which took us a few bugfixes to get right, assuming it fully is now).
I’m hesitating a bit on this one, but it’s probably acceptable, yes. That means consider it’s not a real change, even if it technically is (i.e. assume that everyone which was dealing with those values was applying the right fallback and so that this change does not really change anything in practice).
I think we will eventually want to be able to trust that the database contains the default value (and list properties are not the only type of properties impacted by this problem, by the way), but I also feel like you are quickly not going to like what you embark into if you go down this road. It’s also way too dangerous to backport something like this.
This is totally useless.
This looks like a good first step to me. I’m not sure it’s that difficult to find a way to deal with that fallback in sorting and filtering using various tricks, provided the code in charge of adding the sorting/filtering bit to the query knows about the default value of this property.
It fixes the display issue, but it does not seem to help with the sorting/filtering problems you mentioned for C., since the database is still wrong. More importantly you end up with a loaded XWikiDocument which does not match what’s in the database despite it claiming that it’s not modified. It’s simpler than properly implementing A. but at least as dangerous as A. while not fixing all problems and having the potential of creating a huge mess (so I would argue more dangerous).
It sure is a safe “solution”, but looks like we need to do something
And I think we need to be able to handle this generically since it’s not the last time we will add new user-related xproperties (and the XWikiUsers class can be augmented by users, we even document it as the right way to add custom fields in the user profile). Thus we need to be able to handle xproperties with no values set as we’re not going to migrate all the users every time.
I feel like any solution we’d get would be a lot of complexity for even lower stakes, and it would surely clash with a possible future generic improvement.
I’ll check it out again but as far as I remember it wasn’t that easy. Do you have an example of a similar operation for the sorting/filtering?
Basically just ask Claude code to work on it. If you fix the empty case then you need a custom json page and if you have a custom json page then I don’t think it’s hard to add sorting/filtering.
There’s some example of custom json in the livetable doc but we have that elsewhere too, I’m sure. CC can find it for you.
If the result is not good, drop it but I think you should try to have it and not dismiss it outright.
