Replace concept of User in new User API, and more

Hi devs,

Context

  1. In the new User API we have the concept of User. In practice a User object holds only a UserReference. When calling User#getProperty(propName) it gets the user property on the fly. Thus we don’t really need to have an Object holding a UserReference since we could simply store the UserReference directly and then calls a service to get the user properties.

  2. We need to get the user configuration properties with two use cases:

    • the direct user properties (resolved from the XWiki.XWikiUsers xobject for ex)
    • the user properties with fallbacks on the space preferences, wiki preferences and xwiki.properties config file. For example when asking for the user editor. FTR this is what XWiki#getUserPreferences() currently does but we need we new API for it.

Thus I’m proposing the following new proposal.

New Proposal

  1. We keep the concept of UserReference and UserReferenceResolver.
  2. We drop the User object and UserResolver
  3. We introduce the UserConfiguration object (which represents a typed Configuration Source for user properties).
  4. We use the UserManager to access everything.
    public interface UserManager
    {
        UserConfiguration createUser(UserReference userReference);
        void deleteUser(UserReference userReference);
        boolean exists(UserReference userReference);
        UserConfiguration getXWikiPropertiesUserConfiguration(UserReference userReference);
        UserConfiguration getWikiUserConfiguration(UserReference userReference, boolean inherits);
        UserConfiguration getSpaceUserConfiguration(UserReference userReference, boolean inherits);
        UserConfiguration getUserConfiguration(UserReference userReference, boolean inherits);
    }
    
  5. The Script service $services.user will provide the UserManager API for scripts. It’ll also add a UserReference getCurrentUserReference() API.
  6. We modify the ConfigurationSource API to add setProperty() and save() methods so that the API is not just readonly but also write (and exceptions are thrown for CS that don’t implement the write part such as Composite CS and “xwikipropeties” or “xwikicfg” CS. The implementation should cache the modified properties and keep a list of modified properties and only persist them when save() is called. This allows for batch updates.
  7. Since UserConfiguration implements ConfigurationSource, it’ll also have the write methods. It’ll add typed methods for write (e.g. setDefaultEditor(Editor editor)).
  8. Note: a CS can have parents but when using the write methods, it only affects the curent level (and not the parent).
  9. We continue to allow getting a UserConfiguration object even when the UserReference points to a non-existent user. In this case UserManager#exists() and all the UserConfiguration write methods will throw a UserNotFoundException.

Related

Example usages:

@Inject UserManager userManager;
...
// current user editor with fallbacks
userManager.getUserConfiguration(CurrentUserReference.INSTANCE, true).getEditor()
// current user editor (only from `XWiki.XWikiUsers`)
$services.user.configuration.editor

// current user editor with fallbacks
$services.user.getConfiguration(true).editor
@Inject UserManager userManager;
...
UserReference userReference = ...
UserConfiguration configuration = userManager.createUser(userReference)
configuration.setEditor(Editor.WYSIWYG);
configuration.setType(UserType.SIMPLE);
configuration.save();
  • Note 1: If we agree about the above in principle, we can then work on making it simpler to use the fallbacks by default, for example by having UserManager#getXXXUserConfiguation(UserReference userReference) APIs added that would be equivalents of inherit = true. And invert the script service API so that $services.user.configuration returns the user configuration with fallbacks.
  • Note 2: For simplicity, we could drop the “User” prefixes in the UserManager APIs and have for example:
    public interface UserManager
    {
        UserConfiguration create(UserReference userReference);
        void delete(UserReference userReference);
        boolean exists(UserReference userReference);
        UserConfiguration getXWikiPropertiesConfiguration(UserReference userReference);
        UserConfiguration getWikiConfiguration(UserReference userReference, boolean inherits);
        UserConfiguration getSpaceConfiguration(UserReference userReference, boolean inherits);
        UserConfiguration getConfiguration(UserReference userReference, boolean inherits);
    }
    

This API assume the use can only have one space as parent. Also do we want to have the concept of space in a new API ?

Yes we can tune this. Re space vs Parent Page, yes we can tune this and try to avoid the proliferation of the getXXXConfiguration methods maybe (maybe having 2: one getConfiguration and another one that would allow to get the config from any level). Re Space vs Parent Page I don’t know. ATM Space is not a deprecated concept in APIs ad it’s still fully part of the model.

More importantly to me: do you agree about the proposal in general?

Not sure. Having a User object made sense to me even if it does not contain much right now but this API does not prevent adding it later if we need I guess. Personally I prefer having the get*Configuration APIs in a User object, I’m pretty sure we’ll have more stuff in User at some point.

I’m personally mixed on the general proposal. I’m ok with removing an unnecessary class, but not sure about the names. Basically I’m wondering if we shouldn’t call User the new UserConfiguration class.
I understand that you call it UserConfiguration because it’s a ConfigurationSource, but I’m wondering if it wouldn’t be easier to understand it as a User object for semantic reason.
The first example I have is that UserManager#create returns a UserConfiguration which I find already a bit confusing.

I’m a bit afraid that we’d need the concept of User for future APIs and to be stuck again because we need to break again some APIs because we don’t have it. Now I don’t have real examples of usage right now.

That’s exactly what I proposed on the #xwiki chat last week. It means having a UserResolver return different User objects depending on whether you’re resolving for a given CS or another (and that feels weird). Otherwise it makes the API really not good and forcing to resolve a User to then resolve a configuration, and the User object has no data at all which shows it’s useless and make it overly complex to use for no advantage.

If you check all the places I’ve modified already to use the new User API you’ll see that we never need the User object. So we just retrieve it to get the config data and then we discard it. I haven’t yet found a single use case (even for the user in the xwiki context, we just need the user reference) where we’d need a User object.

The only place where having a User object makes sense is the createUser() API but that represents 1 use case out of thousands and you don’t want to penalize 99% of the use cases for just this one. Moreoever even with createUser() you’re not going to do anything with the User object… since there’s nothing to do with it.

An object is good when it needs to hold state. Gathering all the user configs and storing them in memory in a User object is just stupid, because in all cases you just need to get one or a few config options. It’s much better to do what we do already, ie have a config cache. (In addition it would duplicate the cache)

If you don’t need to hold state then what you need is a Manager/Service. This is what UserManager is about.

I went through the same mental process as you both did… :slight_smile: And it came as a shock to me that se don’t need the User object. What we need now is someone to find a counter example use case where we need it.

Actually there is stuff in the User class which don’t make any sense in the UserConfiguration: user personal info like the name and the mail.

I read again the proposal, and actually I haven’t understood that we wouldn’t have a resolver for UserReference, so this looks overly complicated to me:

I wouldn’t know which one to use to access some basic user informations.

They make sense to me, that’s configuration. You can change your mail, you can change your name. The do not represent the user. The user is represented by its reference.

no no we do have a UserReferenceResolver ofc.

I think you keep mixing UserResolver with UserReferenceResolver :wink:

No it’s definitely not configuration and I can assure you no user will understand why the name of a user have to be gather from UserConfiguration. Anything for which inherit does not make sense have nothing to so in UserConfiguration for me.

1 Like

I didn’t talk about UserReferenceResolver but a resolver for UserReference, so a resolver that would take a reference as input: so I was talking about the UserResolver.

A resolver for UserReference is called a UserReferenceResolver :slight_smile:

vocabulary issue, I was talking about UserResolver