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

[full-ci] - use KQL as default search query language #7212

Merged
merged 14 commits into from
Sep 7, 2023
Merged

Conversation

fschade
Copy link
Contributor

@fschade fschade commented Sep 5, 2023

Description

Use KQL as the default oCIS search query language.

Some examples of a valid KQL query are:

  • Tag: tag:golden tag:"silver"
  • Filename: name:file.txt name:"file.docx"
  • Content: content:ahab content:"captain aha*"

Conjunctive normal form queries:

  • Boolean: tag:golden AND tag:"silver, tag:golden OR tag:"silver, tag:golden NOT tag:"silver
  • Group: (tag:book content:ahab*), tag:(book pdf)

Complex queries:

  • (name:"moby di*" OR tag:bestseller) AND tag:book NOT tag:read

Related Issue

Motivation and Context

Defining a own query language is error prone and leads to a lot of documentation, the oCIS team decided in that ADR to use KQL as our standard. A clear set of rules is set by MS and the dictionary fits well in our usecase.

How Has This Been Tested?

  • unit tests added, have been adjusted
  • acceptance tests have been adjusted

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • ~~Documentation ticket raised: ~~

@fschade
Copy link
Contributor Author

fschade commented Sep 5, 2023

i found one missing part, i'm on it:

Query: "+ID:" + storagespace.FormatResourceID(*info.Id) + ` +Mtime:>="` + utils.TSToTime(info.Mtime).Format(time.RFC3339Nano) + `"`,

@@ -20,7 +20,7 @@ Feature: Search

Copy link
Contributor

Choose a reason for hiding this comment

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

ops, found one deficiency.
You have switched web to use space dav path, but tests still use only the new DAV path. we need to change it for all tests from apiSpaceSearch.feature like:

Scenario Outline: user can find data from the project space
    Given using <dav-path-version> DAV path
    When user "Alice" searches for "*fol*" using the WebDAV API
    Then the HTTP status code should be "207"
    And the search result should contain "4" entries
    And the search result of user "Alice" should contain these entries:
      | /folderMain                                           |
      | /folderMain/SubFolder1                                |
      | /folderMain/SubFolder1/subFOLDER2                     |
      | /folderMain/SubFolder1/subFOLDER2/insideTheFolder.txt |
    Examples:
      | dav-path-version |
      | old              |
      | new              |
    @skipOnRevaMaster
    Examples:
      | dav-path-version |
      | spaces           |

Copy link
Contributor Author

@fschade fschade Sep 5, 2023

Choose a reason for hiding this comment

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

ops, found one deficiency.
You have switched web to use space dav path, but tests still use only the new DAV path. we need to change it for all tests from apiSpaceSearch.feature like:

@ScharfViktor, not exactly sure what you mean, but if you mean that web uses the 'space/*' endpoint for report requests, ...

it's been like that on web master since 5 weeks owncloud/web#9500

I think that also has something to do with the fact that we were on the 7.1 web release in ocis for quite a long time... sorry :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would be nice to have... but it's also not highest prio since we have fallbacks in ocis....

r.MethodFunc("REPORT", "/remote.php/dav/spaces*", svc.Search)

the webdav endpoints won't go away, but we shift our focus to a graph api implementation from now on :)

Copy link
Contributor

@ScharfViktor ScharfViktor Sep 5, 2023

Choose a reason for hiding this comment

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

@ScharfViktor, not exactly sure what you mean, but if you mean that web uses the 'space/*' endpoint for report requests, ...

yes, web uses the 'space/*' but api tests use only new DAV Path. I changed it here 109b70e and I'll create PR
after merging your

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks 😗

@fschade
Copy link
Contributor Author

fschade commented Sep 5, 2023

@2403905, @aduffeck i had to add dateTime support to the kql <-> bleve parser, compiler, and normalizer.

can you please have a look if it's ok for you, specially because of:

Query: "+ID:" + storagespace.FormatResourceID(*info.Id) + ` +Mtime:>="` + utils.TSToTime(info.Mtime).Format(time.RFC3339Nano) + `"`,

we should be ok now, i need another web bump because of:

owncloud/web#9653

can you help @janackermann, @JammingBen

Copy link
Contributor

@aduffeck aduffeck left a comment

Choose a reason for hiding this comment

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

lgtm from reading the code but I haven't had a chance to test it yet.

@fschade fschade marked this pull request as ready for review September 6, 2023 06:45
@fschade fschade requested a review from kulmann as a code owner September 6, 2023 06:45
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

85.9% 85.9% Coverage
0.0% 0.0% Duplication

Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Awesome 👍

@2403905
Copy link
Contributor

2403905 commented Sep 7, 2023

We also have issue with the implicit logical operators
(tag:klass10) NOT tag:physik will be transeted to tag:klass10 AND NOT tag:physik
But
tag:klass10 NOT tag:physik will be transeted to tag:klass10 OR NOT tag:physik

@fschade
Copy link
Contributor Author

fschade commented Sep 7, 2023

@2403905 thats fine! the normalization could be improved, yes, but the current clients are only sending simple queries (i hope this changes soon ;) ), lets close (merge) this here, improve our tests as a first step and then add the implementations step by step....

we first need to know (find out), what is really needed.

@fschade fschade merged commit 844783b into master Sep 7, 2023
@delete-merged-branch delete-merged-branch bot deleted the enable-kql branch September 7, 2023 09:13
ownclouders pushed a commit that referenced this pull request Sep 7, 2023
* enhancement: use kql as default search query language

* enhancement: add support for unicode search queries

* fix: escape bleve field query whitespace

* fix: search related acceptance tests

* enhancement: remove legacy search query language

* enhancement: add support for kql dateTime restriction node types

* chore: bump web to v8.0.0-alpha.2

* fix: failing search api test

* enhancement: search bleve query compiler use DateRangeQuery as DateTimeNode counterpart

* enhancement: support for colon operators in dateTime kql queries
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.

5 participants