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

Fixes #38131 - Use #include? instead of Regexp substring match #940

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ofedoren
Copy link
Member

@ofedoren ofedoren commented Jan 9, 2025

This PR only replaces =~ with include? leaving the current behavior as is.

We could go extra mile with this in terms of optimizing:

    attribute_name = resource_type.constantize.attribute_name
    resource_list = resource_type.constantize.authorized("view_#{resource_type.underscore.pluralize}")
                      .search_for("#{attribute_name} ~ \"#{params[:name]}\"")
                      .limit(100).pluck(attribute_name, :id).map { |name, id| { name: name, id: id } }
    render :json => { :results => resource_list.sort_by { |r| r[:name] }, :subtotal => resource_list.count }

This will take only 100 items from DB which match the param. There are some downsides though:

  1. We lose the exact number of items matching the param to show (since we take max 100 items). To have this back, we'd need to make another call.
  2. Currently it takes items from DB at first and then calls to_s on each. Depending on exact implementation it always will return a string with at least something to compare with the param. This approach uses DB search, so we can compare only string column and if they are present at all. This can be workarounded, but will require more changes. Not sure if it's worth for some entities such as Audit or TemplateInvocation, where Audit.any.to_s produces basically unusable string and TemplateInvocation.any.to_s produces just a string with id of the object, which I don't find useful in context of template inputs. See screenshots:
    Before:
    Screenshot-1736418673663
    Screenshot-1736426440694
    After:

Screenshot-1736426440694w

It'll fail internally with error, since it tries ~ operator on an integer column.

@adamruzicka
Copy link
Contributor

We could go extra mile with this in terms of optimizing:

Could we also sort at the db level to get first 100 resources sorted by that key rather than a random subset of 100 resources which is then sorted?

We lose the exact number of items matching the param to show (since we take max 100 items)

I would be fine with that if we reworded the ui

Not sure if it's worth for some entities such as Audit or TemplateInvocation, where Audit.any.to_s produces basically unusable string and TemplateInvocation.any.to_s produces just a string with id of the object, which I don't find useful in context of template inputs.

I don't really see why anyone would pick an audit or a template invocation so I'd be fine with letting this slide, as long as it works for the more common resources - hosts, subnets and so on

@ofedoren ofedoren force-pushed the ref-38131-regexp-are-nasty branch from 9dbebe0 to 5581e73 Compare January 10, 2025 14:22
@ofedoren
Copy link
Member Author

Could we also sort at the db level to get first 100 resources sorted by that key rather than a random subset of 100 resources which is then sorted?

Added .reorder just in case.

I would be fine with that if we reworded the ui

I've dropped the exact number, but also didn't find any explanation why do we fetch 100 entries to show, so change this magical number into per_page setting. I guess it makes sense to show this number, since it's user dependent and adds a bit consistency with lists such as tables.

As a side effect, since I didn't want to lose the message about more matches, we now fetch per_page + 1, just to check if we have more than per_page matchings to show and, if so, show the message. I guess we can load 1 unused object, since we're dropping even more of unused objects.

Also, I've noticed a weird behavior in selection component, so tried to fix that as well.
Before:
before
After:
after

Please, note, on the second gif, ruby_test item takes a long time before it's selectable even though the server returns it almost instantly. Probably some "artificial" delay caused by a timeout?

@ofedoren
Copy link
Member Author

JS failures seem to be related :/

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

Successfully merging this pull request may close these issues.

2 participants