Replace concept of User in new User API, and more

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

So UserResolver doesn’t exist because there’s no more User. So what we have is a UserConfiguration resolver instead. Either statically in UserManager (which makes all the CRUD operations in one place) or dynamically with a real UserConfigurationResolver as we had with UserResolver. I’m fine with both. For the Script Service API it’s much nicer without the resolve() call but we could still do that even with a resolver I think. But that doesn’t change the User vs UserConfiguration problem.

For me it is. And I don’t see the point of making it more complex than it should be, i.e. making the API a lot more complex because we wouldn’t the ability to override the name or email if not set. We also need this for Guest and SuperAdmin. Also if the user has no name we still need a name for it ('Unnamed" for example or “Unknown User”). Same for mail you can have the use case of defining a global catch all email for your wiki for some use cases. It’s not because you have this ability that you’re going to use it. Right now you can already do this, by creating a first_name xproperty in XWikiPreferences for example, yet it’s not a problem.

So I see it quite interesting because:

  • It unifies the API (same API for all config data). You don’t need think about whether a given property can be overridden or not
  • You can find use cases where you’d want to have the override.
  • It’s when you call that APIs that you decide if you want to get the overrides or not, so depending on your use cases you can decide. But you have the power to have it or not.

I have tried hard to keep the User object over the whole weekend and last week (I spent at least 10 hours just thinking about it and trying various APIs). And I couldn’t find a solution that made sense. There’s just no point of having a User object with state in memory when all you want to get are config data for it. It’s just not efficient and doubles the caches.

What I’m sure of, is that we need an easy API to get user data without getting a User object since that’s the main use case and the User object gets in the way. Whether we also need some User object for some specific case, I don’t know.

So I’m going to reformulate this proposal with the User/UserResolver but with UserConfiguration and UserConfigurationResolver so that at least we have an API to get user data since that’s the main need for now.

The only consequence it would mean for the future, is that a future User object would not need to hold config data since there’ll be the UserConfigurationResolver for them.

Ok?

I’m really not a big fan of using UserConfiguration instead of User, but I guess we can change it later if we need to since it’ll be Unstable, and if you’re stuck because of this, better to move on. so I’m -0 for it

Not a big fan either but as long as there is no more debatable entry points like UserManager and create() that return a UserConfiguration it should not be a big blocker for the future so fine with me.

Instead of UserConfiguration I prefer to use UserProperties for 3 reasons:

  • UserConfiguration is already used and it follows the current best practice naming pattern of being the configuration for the User module.
  • The API (inherited from CS) is getProperty() so we’re retrieving user properties. It’s consistent.
  • It would solve @tmortagne’s point above about some properties not being configuration. For example “first name” is a user property for sure. Whether it’s inheritable or not is another matter but from an API POV it’s not a problem to ask for it through UserProperties.

Proposal:

// Get all user properties, with inheritance (internally using the "all" CS)
@Inject
@Named("all")
private UserProperties userProperties;
...
userProperties.resolve(CurrentUserReference.INSTANCE).getEditor();

// Get direct user properties (ie from XWikiUsers for the "document" implementation), without inheritance (internally using the "user" CS)
@Inject
@Named("user")
private UserProperties userProperties;

// Get user properties from the current space, with inheritance (internally using the "user" CS)
@Inject
@Named("space")
private UserProperties userProperties;

Note: it’s important to be able to get the UserPreferences instance at each level for the write use case (to be able to set properties, see my first post in this thread about the write API).

Scripting API:

// Get all user properties, with inheritance (internally using the "all" CS)
$services.user.allProperties.editor

// Get direct user properties (ie from XWikiUsers for the "document" implementation), without inheritance (internally using the "user" CS)
$services.user.directProperties.editor

// General API
$services.user.getProperties("<UserProperties hint>").editor

Documentation updated:

Hi,

(admittedly scanned rather superficially the discussion, but:)

The current proposal looks very twisted to me. The whole point about the new user API (from my POV) was to have Users as first class citizens in the XWiki API and stop handling them as consequences (objects in pages with properties, etc.) rather than design.

What was in my mind initially and what makes sense to me, after quickly reading the above is something like this:

  • User class (first class citizen in the API). Probably better to be an interface, but example:
public class User {
  private UserProperties properties;
  private UserReference reference;
  public getReference();
  public get/setProperty(...) { properties.get/set(...) }
    or
  public getProperties() { return properties } <-- uses getter/setter on UserProperties instead of on User which also allows for typed getters
}

Note: UserProperties seems to have some known typed getters, but would probably also need an untyped getter/setter, for extensibility (AFAICS, it already does not have typed getters for all the default properties we have in the XWikiUsers xwiki class).

  • UserManager: performs CRUD (create/get/save/delete) on User objects (also going into the UserProperties and saving to DB any changes that might have been done in the live UserProperties object).
interface UserManager {
  User create(UserReference reference);
  User get(UserReference reference);
  void delete(UserReference reference);
  void save(User user);
  /* Optional, convenience/optimization methods: */
  boolean exists(UserReference reference);
  User getCurrent();
  Object getProperty(UserReference reference, String propertyName);
  void setProperty(UserReference, String propertyName, Object value);

Various implementations of UserManager would produce various implementations of User. Not sure we need a UserManagerProvider as well, in order to openly and easily accommodate for alternative and configurable UserManager implementations which would replace the default (XWikiDocument based) one or if we should just rely on the component overriding mechanism (which is a bit more intrusive).

UserConfiguration sounds really bad to me because the main things about a user’s data are his profile information (name, description, website, phone number, etc.). UserProperties sounds a bit better, indeed, and can also accept things that might be regarded as “configuration”, like what editor to use, etc.

Yes, technically, a user is mostly just a reference and a bunch of properties around it, but we could have said the same thing about Document or any other API citizen. You need a class to pull it all together and to also help the API callers better wrap their minds around them, on familiar terms. I’m sure we could be able to optimize any caching issues in the background. Again, I don’t see much of a difference between User and Document (XWikiDocument).

It has already been said that creating users is complicated/unnatural, but I’d also add the even getting a couple of users and putting them in a collection would result in getting a couple of UserProperties and losing them in the collection, since the UserProperties object does not seem to link back to the UserReference that it corresponds to. Sure, we could probably fix this with a new getter/setter, but it feels so awkward and it would have been much more appropriate to handle Users instead.

The entire resolver logic and the handling of superadmin/guest got me running around places while looking at the default implementation (https://github.com/xwiki/xwiki-platform/tree/master/xwiki-platform-core/xwiki-platform-user/xwiki-platform-user-default). Overall, I had the deep impression that it’s overly complicated.

I’m also -0. I’m not available to work on it, so I could not impose for more, as I would have otherwise leaned more towards -1.

I’m sure I’m missing things, as I did not sit on the topic enough (with respect to Vincent’s work so far). Just wanted to share my POV, as the current proposal looks unnecessarily complex to me and not natural to use. (…or maybe it’s just not what I had expected, don’t know). Hoping you might get some new ideas out of it.

Thanks for your work, Vincent. I’ve been waiting for a long time for a users API.