[fschade]

Description

Two things showed up as search results that nobody looks for:

• the root of a personal space (it is named after the owner, so searching that name returned it)
• the .space folder with the space image and readme.

both stay in the index on purpose, we just do not return them as a result.

Every search now runs through a default KQL template, so it takes effect on existing indexes without a reindex for both engines. {{.QUERY}} is where the user's own search goes, the rest is the default constraint around it.

The default ({{.QUERY}}) AND (NOT path:"./.space" AND NOT id:{{.SPACE_ROOT_PERSONAL}}) takes care that no resource within .space/* and no personal space root is part of the search result!

the following 3 placeholders are available:

• {{.QUERY}} the user's own search, wrapped as ({{.QUERY}}), required exactly once
• {{.SPACE_ROOT_PERSONAL}}
• {{.SPACE_ROOT_PROJECT}}

The design is not limited to space roots: more placeholders can be added, and the template can carry any default constraint a deployment wants on every search, narrowing (AND) or widening (OR).

please note, {{.}} does not use any template engine (yet), its here to be forward compatible in the future!

The main focus of the PR is to be able to disable (hide) certain results, but it can also be used to enhance or reduce these:

• introduce a tag called invisible, then add that tag as NOT in the template vie conf
• always add certain resources to the set of results OR (some guidelines or similar)

How Has This Been Tested?

• tests

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
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation added

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 22 complexity

Metric	Results
Complexity	22

View in Codacy

🟢 Coverage 90.00% diff coverage · +0.15% coverage variation

Metric	Results
Coverage variation	✅ +0.15% coverage variation (-1.00%)
Diff coverage	✅ 90.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (3826240)	92399	23478	25.41%
Head commit (9d972f1)	92480 (+81)	23639 (+161)	25.56% (+0.15%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3787)	120	108	90.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[dschmidt]

Not against this per se, just wondering if there's an easier solution to the problem you're trying to solve:
Isn't .space hidden? Would it make sense to make hidden=false the default for search requests?
And also simply filter out (personal) space roots?

I'm a bit wary of the new config surface especially with the placeholders in it.

What are other use cases for the config?

edit:
Most importantly I'm wondering what should be configurable and what should be part of a default configuration.

I could imagine hidden=false being a nice default config parameter, it's not overly specific. It's something that most people will like this way, but switching it around is easy enough. I would never expect the personal space root as a search result, I'm not sure we need to enable users to allow that.

[dschmidt]

One more thing, separate from the placeholder discussion: balancedParentheses and the recover() in compiles reimplement what our KQL parser should be doing, and I think they exist because of two gaps in the grammar itself.

I fed kql.Builder{}.Build a few inputs that should all be rejected:

input	what happens	why that is a problem
NOT path:x) OR (name:y	parses as NOT path:x, the rest is silently dropped	the user gets results for a different query than they typed, with no error
NOT	parses as a single operator node	a lone operator has nothing to negate, the compilers have to guard against it
foo AND	parses as foo, AND	a dangling binary operator is accepted, the compilers have to guard against it
foo AND NOT	parses as foo, AND, NOT	same, plus the NOT has nothing to negate
(NOT)	parses as a group with a lone operator	same, inside a group

Two causes in pkg/kql/dictionary.peg: AST has no EOF anchor, so anything after a valid prefix is ignored, and Node lists the boolean operators as standalone alternatives in a flat (_ Node)+ sequence, so an operator never needs an operand. Both already hit user queries today, and the first one undermines the guarantee of this PR: a user query like foo) OR (name:alice becomes (foo) OR (name:alice) AND (NOT ...) after wrapping, and the foo branch escapes the filter.

I'd rather fix that in the grammar than scan strings in the config parser. A positional shape that still emits the same flat node list (so connect.go and both compilers stay untouched):

AST     <- _ n:Nodes _ EOF
Nodes   <- Operand (_ (OperatorBooleanBinaryNode _)? Operand)*
Operand <- (OperatorBooleanNotNode _)* (GroupNode / PropertyRestrictionNodes / FreeTextKeywordNodes)
EOF     <- !.

That removes balancedParentheses and the recover() in one go, with one condition: both sides have to be parsed on their own before they are concatenated, because the wrapped string is a valid query in both escape cases ((foo) OR (name:alice) AND (...) and (x) AND (a) OR (b)). For the filter that is a standalone kql.Builder{}.Build at startup, engine neutral and without the query/bleve import in the config package. For the user query it is one parse in the service before withQueryFilter, which PinnedRootID already does anyway and currently ignores errors from. A balanced expression wrapped in parentheses is then always safe.

Side effect of the grammar change: a leading AND/OR becomes a parse error instead of StartsWithBinaryOperatorError, so the nine test cases pinning that type need adjusting and validateAst loses that branch.

Validating with the parser instead of the bleve compiler also closes an engine gap: right now the check runs the bleve compiler only, and the OpenSearch transpiler rejects some nodes bleve silently skips (number or date without operator), so a filter like NOT size:5 would pass startup on OpenSearch and then fail every search.

I did consider parsing the filter once and splicing AST nodes, or doing it in the Normalize pass. Neither is worth it: the engine interface takes a query string and both engines parse themselves, and Normalize has neither the config nor the space type. Parsing a 60 char string per search is microseconds next to the index lookup. The string splice with parentheses is precedence safe once both sides are parsed on their own, so withQueryFilter can stay as is.

[dschmidt]

Not against this per se, just wondering if there's an easier solution to the problem you're trying to solve: Isn't .space hidden? Would it make sense to make hidden=false the default for search requests? And also simply filter out (personal) space roots?

I'm a bit wary of the new config surface especially with the placeholders in it.

What are other use cases for the config?

edit: Most importantly I'm wondering what should be configurable and what should be part of a default configuration.

I could imagine hidden=false being a nice default config parameter, it's not overly specific. It's something that most people will like this way, but switching it around is easy enough. I would never expect the personal space root as a search result, I'm not sure we need to enable users to allow that.

Using hidden doesn't work - because what we exclude in the config, cannot be "re-included" in a user query. I.e. if we add hidden=false to the configurable query, users cannot find any hidden files or files in hidden folders anymore. That certainly wasn't my intention.

The placeholders are simple enough and as I see at least a small benefit in keeping them, I'm ok with adding them even if it means we can probably never remove them again :)

[dschmidt]

Great, thanks! LGTM.

Just a few nitpicks:

• SyntaxError.Error() forwards the whole pigeon error list (no match found, expected: "(", ")", "+", ...) into the bad request body. Not wrong, but loud for a client facing message. A shorter text plus the position would do.
• The user query is now parsed three times per request: once in Search for validation, once in PinnedRootID, once in the engine. Microseconds, but PinnedRootID could take the AST from the first parse instead of the string.
• The placeholder NewReplacer is duplicated between parse.go and service.go. One expander would do, the config parser can call it with an empty space.
• The leading _ in AST <- _ n:Nodes _ EOF is redundant, Nodes already starts with _. Harmless.
• Commit subject is short now but still not imperative (CONTRIBUTING asks for "add ..." style).

[fschade]

@dschmidt thanks for all the feedback, really helpfull!

[fschade]

@Svanvith FYI A KQL template every search runs through, see the search service documentation for the available placeholders

[butonic]

Nice!