Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add full_text_aspect liquid filter #7357

Merged
merged 3 commits into from
Oct 24, 2020
Merged

Conversation

deanmarcussen
Copy link
Member

Fixes #7128

Adds a full_text_aspect filter, which can be used in the FullTextSettings to allow Widgets or Bags items contribute towards the full text search index.

Usage

{{ Model.Content.FlowPart.Widgets | full_text_aspect }}

or 

{% for contentItem in Model.Content.FlowPart.Widgets %}
  {{ contentItem | full_text_aspect }}
{% endfor %}

@deanmarcussen deanmarcussen requested a review from Skrypt October 22, 2020 16:58
@sebastienros
Copy link
Member

Potential stack overflow to fix

@deanmarcussen deanmarcussen requested a review from jtkech October 24, 2020 12:11
@deanmarcussen
Copy link
Member Author

deanmarcussen commented Oct 24, 2020

@jtkech might ask you to take a look at this, as I know you did a shape recursion pr a while back.

Here I have to use the ContentItemId to evaluate against as there is too much JObject.ToObject to use object equivalency.

I also found it possible to stackoverflow a liquid Content template, by recursively building displays.

Could not find a way to break it with any of the other helpers.

Edited
Refactored the helpers a bit :)

@jtkech
Copy link
Member

jtkech commented Oct 24, 2020

@deanmarcussen

@jtkech might ask you to take a look at this, as I know you did a shape recursion pr a while back.

Okay, i will take a look this night

@agriffard agriffard merged commit 491d60b into dev Oct 24, 2020
@delete-merged-branch delete-merged-branch bot deleted the deanmarcussen/fulltext_aspect branch October 24, 2020 20:46
services.AddLiquidFilter<FullTextFilter>("full_text");

services.AddScoped<IContentItemRecursionHelper<FullTextFilter>, ContentItemRecursionHelper<FullTextFilter>>();
services.AddScoped<IContentItemRecursionHelper<BuildDisplayFilter>, ContentItemRecursionHelper<BuildDisplayFilter>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have registered a generic one

services.AddScoped(typeof(IContentItemRecursionHelper<>), typeof(ContentItemRecursionHelper<>));

So that another one could be resolved on demand without having to do another registration

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that @jtkech didn't even know about it.

Will do some docs shortly, so will change that then.

{
if (_contentItemIds.Contains(contentItem.ContentItemId))
{
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes i just cache the whole object and contains uses a by ref comparison

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I couldn't because there is too much JObject.ToObject<ContentItem> which breaks by ref

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool then

// When {{ Model.ContentItem | shape_build_display | shape_render }} is called prevent recursion.
if (buildDisplayRecursionHelper.IsRecursive(contentItem))
{
return new ValueTask<FluidValue>(NilValue.Instance);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure here that it can't be called more than once, not necessarily through the same model / shape, but on the same item, this in a given request scope?

@jtkech
Copy link
Member

jtkech commented Oct 24, 2020

@deanmarcussen

Too late, already merged ;) I still did a little review

Yes i did some recursions checking, as i remember it was to prevent a shape / model to render itself too many times, by comparing by ref the passed model to the current one, max recursions being equal to 3. The use case is

{{ Model | shape_render }}

I still did a little review, the main remark is to register a generic helper

services.AddScoped(typeof(IContentItemRecursionHelper<>), typeof(ContentItemRecursionHelper<>));

One question, the recursion helper is also used in the BuildDisplayFilter, are we sure that we will never need to render more than once, not necessarily through the same model / shape, but the same content item? This in a given rq scope.

@deanmarcussen
Copy link
Member Author

One question, the recursion helper is also used in the BuildDisplayFilter, are we sure that we will never need to render more than once, not necessarily through the same model / shape, but the same content item? This in a given rq scope.

Tricky @jtkech , in a single request scope if you accidentally call | shape_build_display | shape_render on the same content item as the template you are in, you'll always end up back in that template.

So preventing that.

But it is possible you might want to render items twice.
Perhaps in a different display mode.

I have done that sometimes for a list view and a tile view... But in Razor.

Hmm I will think if there is a better way to keep it safe, but not preventing scenarios either.

@jtkech
Copy link
Member

jtkech commented Oct 27, 2020

@deanmarcussen Yes i meant in the same request scope, so now in the same template context, but from 2 different models / shapes but displaying the same content item, e.g. in 2 different places, yes maybe in 2 display modes.

But that's okay for me ;)

Otherwise maybe just a max recursion counter with a max recursion value

Hmm, one solution would be to also pass to your helper the current Model (accessible by any liquid filter) and then in your helper having a set of ContentItemId but per model e.g. with a dictionary of <object, HashSet<string>>, hmm if the model can be null maybe with a null key if it is possible. Then you could avoid the 1st recurssion under the same model (here you can compare 2 model by ref, this is what i did to check our other max shape recurssion)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full Text Index for Widgets/Bag Items
4 participants