umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.42k stars 2.67k forks source link

`ToPublishedSearchResults` doesn't work for members #12891

Open bjarnef opened 2 years ago

bjarnef commented 2 years ago

Which exact Umbraco version are you using? For example: 9.0.1 - don't just write v9

10.0.1

Bug summary

Currently there is a ToPublishedSearchResults() Examine extension method, which works for Content and Media, but not when querying Members Index. https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/src/Umbraco.Infrastructure/Examine/ExamineExtensions.cs#L92

The following does return results, so it would be useful to get the results as IEnumerable<IPublishedContent> similar to when extracting data from MTNP or Member Picker using property value converters.

var results = query.Execute(new QueryOptions(skip, take));

Either using this:

var items = results
     .ToPublishedSearchResults(umbracoContext.PublishedSnapshot)
     .Select(x => x.Content);

or this, where umbracoContext.PublishedSnapshot.Members is IPublishedMemberCache.

var items = results
     .ToPublishedSearchResults(umbracoContext.PublishedSnapshot.Members)
     .Select(x => x.Content);

Specifics

No response

Steps to reproduce

Query Examine member index and try get member results using ToPublishedSearchResults() extension method.

Expected result / actual result

No response

github-actions[bot] commented 2 years ago

Hi there @bjarnef!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

bjarnef commented 2 years ago

A workaround it to use something like this, although it lookup members similar to how it is handle here in MemberPickerValueConverter: https://github.com/umbraco/Umbraco-CMS/blob/e626fca2432582f052cb13654eedd9e60ef8723f/src/Umbraco.Core/PropertyEditors/ValueConverters/MemberPickerValueConverter.cs#L65-L99

var umbracoContext = _umbracoContextAccessor.GetRequiredUmbracoContext();

var results = query.Execute(new QueryOptions(criteria.Skip, criteria.Take));

List<IPublishedContent> items = new();

foreach (var result in results)
{
    IMember? m = int.TryParse(result.Id, out int id)
        ? _memberService.GetById(id)
        : null;

    if (m == null)
        continue;

    var member = umbracoContext.PublishedSnapshot?.Members?.Get(m);
    if (member == null)
        continue;

    items.Add(member);
}

return new PagedResult<IPublishedContent>(results.TotalItemCount, criteria.Skip + 1, criteria.Take)
{
    Items = items
};

However it not possible to get full reflection of member as IPublishedContent it would be great to get a slim version of it. Most of the basic properties are available in Member Index already.

bjarnef commented 2 years ago

@nul800sebastiaan is there a simple way to make ToPublishedSearchResults works for members as well?

Maybe a different overload of this. https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/src/Umbraco.PublishedCache.NuCache/MemberCache.cs#L36-L44

nul800sebastiaan commented 2 years ago

Currently this not supported.

                    case IndexTypes.Member:
                        throw new NotSupportedException("Cannot convert search results to member instances");

No idea if it can be though or why it's not supported.

nul800sebastiaan commented 2 years ago

At the time this seemed like a bad idea apparently: https://github.com/umbraco/Umbraco-CMS/pull/10159#discussion_r618306984

bjarnef commented 2 years ago

@nul800sebastiaan yeah.. there isn't really a member cache like content and media. Indexing of members loop IMember instances and member picker seems to use IMember as well.

Maybe we shouldn't use IPublishedContent, but maybe another "slim" model. But in that case it could be a different utilty method. Perhaps @bergmania or @Shazwazza has some input on this?

I can of course create my own model and map the properties in the specific project, but ideally it would be nice to work with the generated ModelsBuilder member model for in the paged result. But to create instances of e.g. Member it need to pass in IPublishedContent.

Shazwazza commented 2 years ago

See https://github.com/umbraco/Umbraco-CMS/pull/10159#issuecomment-1251177259

bjarnef commented 2 years ago

Often we have a paged result (I don't recall the exact default) but it could of course be increased to I large number, which in that case would perform bad.

Maybe we need a different utility method the deal with member search results either using MemberManager, mapping properties to a dictionary, or a slim entity (I think Id, Key and Name is always available, while additional properties could be mapped to a dictionary).

Alternatively we could write some documentation to cover this and suggestion to convert the member search results to a strongly typed model.

We could of course write our own, but would be great to share the thoughts with the community.

Shazwazza commented 2 years ago

I there were a method to bulk fetch members with IMemberManager than it could all work fine. The current problem is having to lookup individual members one at a time (which was how it worked in v8 too). Then you can have an extension method somewhere that does this:

deekoulos commented 2 years ago

@bjarnef is right, you usually load paged data, so performance shouldn't be an issue. Please give us the option of bulk loading the members directly and not first the ids and then the bulk lookup. As already mentioned, performance should be fine.

We also have a lot of custom properties applied to the member type. If using the memberManager, would we also have access to all of the member's custom properties?

Is there currently any method that can convert the .AllValues property of the search results? This is our workaround for loading a member from the index by some certain values:

var member = index
            .Searcher
            .CreateQuery(IndexTypes.Member)
            .ManagedQuery(key, fields)
            .Execute(new Examine.Search.QueryOptions(0, 1))
            .Select(x => JsonConvert.DeserializeObject<MemberViewModel>(JsonConvert.SerializeObject(x.Values)))
            .Single();

I don't really like what we are doing with the serialization and deserialization in the above code which costs us more operation time. But what would be here the alternative with the current version?

In v8 we did here x.Content as Member and all was fine.

Shazwazza commented 2 years ago

@bjarnef is right, you usually load paged data, so performance shouldn't be an issue.

I don't think you are following me here.

The problem that exists in v8 is that you do a paged Examine query (which you are doing in your example) and behind the scenes, for each result, individually, it will make a call to the IMemberService.GetById. So if you have a page size of 20, you are making 20 individual service calls which will probably equate to about 50+ DB queries and most of them are expensive.

It's not about paging, it's not about Ids, its about the N+1 problem.

No matter what, we are querying members by Examine (i.e. IContentQuery.Search). This returns Examine results, we need to take those results and lookup the real member in the DB because the indexed data will be different in a lot of cases and then transform that member to IPublishedContent. Again, this is what happens in v8. In v9+, you can do the exact same thing but it will be manual with an IMemberManager lookup and then a call to IMemberManager.AsPublishedMember to transform to IPublishedContent.

Moving forward, there just needs to be a method to bulk lookup members in IMemberManager by multiple Ids so we don't have an N+1 issue just like I mention above.

We also have a lot of custom properties applied to the member type. If using the memberManager, would we also have access to all of the member's custom properties?

Yes of course

Is there currently any method that can convert the .AllValues property of the search results? This is our workaround for loading a member from the index by some certain values:

No, and it will not be possible because indexed values may not be the same as what is stored and you cannot run IPropertyValueConverters here.

In v8 we did here x.Content as Member and all was fine.

Again - behind the scenes this does an N+1 lookup, it will use IMemberService.GetById for each result, this is not good and removed for performance reasons. Even though you are not seeing performance issues, others will. It will depend on the load of that search page (RPS).

bjarnef commented 2 years ago

@Shazwazza it seems IMemberManager inheriting IUmbracoUserManager<MemberIdentityUser> currently doesn't have a method to find multiple members by ids.

I guess it would be possible to extend the interface e.g. IExtendedMemberManager inheriting some of the core interfaces or maybe an extension method on the interface.

However I found Umbraco inherits the FindUserAsync() method here: https://github.com/umbraco/Umbraco-CMS/blob/e626fca2432582f052cb13654eedd9e60ef8723f/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs#L88

https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.identity.userstorebase-5.finduserasync?view=aspnetcore-6.0

I guess the Users property would be used to query multiple users/members by ids? https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.identity.userstorebase-5.users?view=aspnetcore-6.0

@Zeegaan @bergmania could we implement a method to query multiple users/members by ids in IMemberManager with default implementation to avoid breaking change. Not sure if it possible the extend the current implementation to lookup multiple members without looking up individual member.

bjarnef commented 1 year ago

Would it be possible to query multiple members/users in IMemberManager and IUmbracoUserManager. I seems it is possible to query users https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.identity.iqueryableuserstore-1.users?view=aspnetcore-7.0 but I guess this is what's not implemented here: https://github.com/umbraco/Umbraco-CMS/blob/e626fca2432582f052cb13654eedd9e60ef8723f/src/Umbraco.Infrastructure/Security/UmbracoUserStore.cs#L25

kjac commented 1 year ago

Hi @bjarnef šŸ‘‹

We've had a talk about this today... long story short - we won't be taking steps to mend this.

That being said, if you or someone else can build something that is:

  1. Performant
  2. Secure (not leaking e.g. role bindings unless explicitly instructed to)
  3. Compliant (not leaking personal data unless explicitly instructed to)
  4. Non-breaking (or targeting V13)

...well then we'd of course be more than happy to have a look at that šŸ˜†

In other words: This one goes up for grabs for a bit. Let's see if someone jumps at it.

bjarnef commented 1 year ago

@kjac fair enough. However I think the IMemberManager and IUmbracoUserManager should implement method to allow querying multiple users. As @Shazwazza mentioned here it would be much more performant, that look up each member/user individually: https://github.com/umbraco/Umbraco-CMS/issues/12891#issuecomment-1251502369