Skip to content

search: don't fail on search if regexp pattern is not compatible with db dialect

Administrator requested to merge backend-integration/rvt/valid-repo-regex into main

Created by: rvantonder

Fixes https://github.com/sourcegraph/sourcegraph/issues/28781.

This follows the approach I mentioned in the issue, which is to validate up front whether a pattern can be used for a repo search, and only creates a repo search job if true. This does mean that valid patterns are potentially parsed twice by the DB parse function for repo search. This is small price to pay for making the semantics less broken, and is actually a side effect of the fact that our query pipeline along the repo search code path is inverted from how we should structure it (like the rest of our query code), which is:

  • Parse a data structure -> do validation up front -> fail if needed -> otherwise created valid internal representation, no more static validation here

(note validation is only done once)

By doing the validation up front for the repo search path, this PR establishes where the "right" place is to do this check (i.e., before performing any other real work). Ideally, what we want, is to also create the sqlf query up front, once we know that the regexp is valid, and that we will run a repo search job. In other words, ideally the repo search job will take the already-ready-and-validated DB repo query. I'm not investing myself in that effort right now, the repo code path is a different beast that I'll target another day maybe. Just laying out why this change follows a consistent and predictable architecture.

For repo search specifically, the current behavior is still imperfect because ideally, if a user ran only a repo search, and expected a \s to match a repo name (unlikely, but possible), they would get 'no results' instead of a validation error (like "\s unsupported for repo search"). But this PR takes steps to at least be less conservative and allow the rest of a search to succeed.

Merge request reports

Loading