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

[dev.icinga.com #11160] Please align URL capabilities to filters #2296

Closed
icinga-migration opened this issue Feb 15, 2016 · 14 comments
Closed
Labels
area/framework Affects third party integration/development bug Something isn't working

Comments

@icinga-migration
Copy link

This issue has been migrated from Redmine: https://dev.icinga.com/issues/11160

Created by tgelf on 2016-02-15 11:31:39 +00:00

Assignee: (none)
Status: Feedback
Target Version: Backlog
Last Update: 2016-12-22 15:44:00 +00:00 (in Redmine)


We introduced technical debt by implementing filter capabilities with no matching URL rendering rules. We must bring our filters back to a state where all of them can be rendered to and parsed from URL strings. We should also take care that most of our data providers using filters are able to render/understand all filter capabilities.

Cheers,
Thomas

@icinga-migration
Copy link
Author

Updated by elippmann on 2016-02-18 11:35:42 +00:00

  • Status changed from New to Feedback
  • Assigned to set to tgelf

Hi Tom,

Could please explain this in more detail. I don't know what you mean :)

Best,
Eric

@icinga-migration
Copy link
Author

Updated by tgelf on 2016-02-18 12:13:30 +00:00

We added for example case-sensitiveness and afair array matches or similar. I expect a test doing Filter::fromQueryString($handCraftedFilterObject->toQueryString()) to always build the very same original object, no matter in what way you built your $handCraftedFilterObject.

Cheers,
Thomas

@icinga-migration
Copy link
Author

Updated by elippmann on 2016-02-18 14:04:31 +00:00

Do you have a quick example where this is not the case?

@icinga-migration
Copy link
Author

Updated by tgelf on 2016-02-18 14:39:32 +00:00

We already slightly broke the rules with FilterMatch in #6557. A dedicated sign like the mentioned ~ has never been introduced, so FilterMatch renders like FilterEqual - no way to find back. That's not a problem, as they have been separated "for future improvements".

But now we have FilterMatchNot, FilterMatchNotCaseInsensitive, FilterMatchCaseInsensitive - and I see no corresponding code that takes care of rendering / parsing of those.

Additionally implemented filters should be supported in our most important filterable backends (DB, LDAP...). There is also magic array-handling support that should be defined and tested. If I correctly read the code 809861c (just an example) changed existing behaviour as it probably broke a=(b|c*). Please note that I didn't test this, but to me it looks like it returns too early to let the former logic happen. So there is either a bug or dead code to be removed. (but that's not what this issue is all about, I stumbled over that commit while researching for this answer).

@icinga-migration
Copy link
Author

Updated by elippmann on 2016-02-19 15:25:53 +00:00

  • Category set to Framework
  • Status changed from Feedback to New
  • Assigned to deleted tgelf
  • Target Version set to Backlog

@icinga-migration
Copy link
Author

Updated by aklimov on 2016-12-22 09:18:17 +00:00

  • Status changed from New to Assigned
  • Assigned to set to aklimov

@icinga-migration
Copy link
Author

Updated by aklimov on 2016-12-22 14:13:21 +00:00

  • File added new-filter-syntax.md
  • Status changed from Assigned to Feedback
  • Assigned to changed from aklimov to jmeyer

I think it would be better to rebuild the URL syntax completely to make it more "alignable". See the attached file for details.

@icinga-migration
Copy link
Author

Updated by tgelf on 2016-12-22 15:27:36 +00:00

Replacing such a central component is not what this issue asked for. You do not build a new house when asked to paint it's windows, do you? Sorry, no way at this point. You might want to open a dedicated issue for your proposal. Filter parsing is used for URLs, restrictions, shared navigation filters, many places in Director and other modules. Good luck with migrating all of those.

This issue should be fixed for our existing Filters, regardless of whether we implement five new better implementations or not.

@icinga-migration
Copy link
Author

icinga-migration commented Dec 22, 2016

Updated by aklimov on 2016-12-22 15:44:00 +00:00

  • Assigned to deleted jmeyer

tgelf wrote:

You do not build a new house when asked to paint it's windows, do you?

No, but I feel free to suggest building a new house if this seems far easier than painting that paticular windows for me. That's what I did – no more and not less. (FYI: I even haven't written a single line of code for this proposal.)

tgelf wrote:

You might want to open a dedicated issue for your proposal.

Fine. #13741

tgelf wrote:

Filter parsing is used for URLs, restrictions, shared navigation filters, many places in Director and other modules. Good luck with migrating all of those.

Who says "migrating"?

aklimov wrote:

The old syntax may still be supported not to break existing links using filters (...)

I could imagine that the old and new syntaxes can coexist even in the same filter.

@icinga-migration
Copy link
Author

Updated by aklimov on 2016-12-22 15:44:06 +00:00

  • File deleted new-filter-syntax.md

@icinga-migration icinga-migration added feedback queue/important Blocks a release or needs immediate attention bug Something isn't working area/framework Affects third party integration/development labels Jan 17, 2017
@icinga-migration icinga-migration added this to the Backlog milestone Jan 17, 2017
@Thomas-Gelf
Copy link
Contributor

Interesting, thanks. So now that we can track that proposal in a dedicated issue, may I ask for the state of the of the original request? 👅

@lippserd
Copy link
Member

Nothing happened I guess :)

@dnsmichi dnsmichi added needs-feedback We'll only proceed once we hear from you again and removed feedback labels Nov 9, 2017
@Al2Klimov Al2Klimov self-assigned this Feb 6, 2018
@Al2Klimov Al2Klimov changed the title [dev.icinga.com #11160] Please align filters to URL capabilities [dev.icinga.com #11160] Please align URL capabilities to filters Feb 6, 2018
@Al2Klimov
Copy link
Member

Suggestion:

  • a*=b: case insensitive modifier for match
  • a~=b: equal instead of match
  • a*~=b: case insensitive equal
  • a!*=b: case insensitive modifier for not match
  • a!~=b: not equal instead of not match
  • a!*~=b: case insensitive not equal

Additional ideas:

  • a>(b|c|d) and any other operator in addition to a=(b|c|d)
  • a=(b&c&d) in addition to a=(b|c|d)

@Al2Klimov Al2Klimov assigned lippserd and unassigned Al2Klimov Feb 6, 2018
@lippserd lippserd removed this from the Backlog milestone Apr 11, 2018
@nilmerg nilmerg removed needs-feedback We'll only proceed once we hear from you again queue/important Blocks a release or needs immediate attention labels May 2, 2019
@nilmerg
Copy link
Member

nilmerg commented Oct 16, 2020

Since nothing happened after soon 5 years, we can forget about this I think. Though, it's not only that nothing has happened:

  • Anyone using FilterMatch directly did this with an equal comparison in mind, as that's also what's being used for Filter::expression('foo', '=', 'bar'), so FilterEqual is the alien here and is completely redundant.
  • Case insensitive filters were never meant to be rendered back to a string representation, as that would allow the end user define the case sensitivity. Now think about allowing the user to perform a case insensitive search over customvars or plugin output, without having an index reflecting the necessary collation. (Not mentioning the necessity to know prior performing a query which column even supports case insensitivity.)
  • Array matching didn't broke with the mentioned commit. Maybe another commit broke (foo|ba*), or it never has worked. Supporting this for any backend would also be difficult if not impossible.

And since I'm about to implement an alternative to Icinga\Data\Filter in the ipl, which still uses the very same string representation, this case can be closed once and for all.

@nilmerg nilmerg closed this as completed Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/framework Affects third party integration/development bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants