Page MenuHomePhabricator

Add repositories to fulltext search index.
ClosedPublic

Authored by 20after4 on Feb 2 2017, 6:34 PM.
Referenced Files
F12846878: D17300.id42262.diff
Fri, Mar 29, 1:36 AM
F12834734: D17300.id42246.diff
Thu, Mar 28, 2:47 PM
F12833116: D17300.diff
Thu, Mar 28, 1:52 PM
Unknown Object (File)
Wed, Mar 20, 10:24 PM
Unknown Object (File)
Mon, Mar 18, 7:06 AM
Unknown Object (File)
Mon, Mar 18, 7:06 AM
Unknown Object (File)
Mon, Mar 18, 7:06 AM
Unknown Object (File)
Mon, Mar 18, 7:06 AM
Subscribers

Details

Summary

This implements a simplistic PhabricatorRepositoryFulltextEngine
Currently only the repository name, description, timestamps and
status are indexed.

Note: I had to change the search index workflow to disambiguate
PhabricatorRepository from PhabricatorRepositoryCommit

Test Plan
  • ran ./bin/search index --type PhabricatorRepository --force
    • searched for some repositories. Saw reasonable results matching on either title or description.
  • Edited a repository in the web ui
    • Added unique key words to the repo description.
    • I was then able to find that repo by searching for the new keywords.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Looks good. Couple of minor nits inline.

Your test plan should also include something like this:

  • Edited a repository to have "platypus" in the description, searched for "platypus", found the repository.

Even when bin/search index works, the actual "edit stuff with the web UI" workflow may still not be fully hooked up. Usually, this is because the Editor does not override supportsSearch(), so edits don't rebuild the index. In this case it looks like you're in the clear, so I'd expect this to work correctly with no further changes, but it's worth double checking.

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
10

This kind of documentation is unconventional in this codebase, and the docs say $repository while the actual variable is $repo. I think this is perfectly clear from context anyway, just nuke it?

15

Newline is probably a better delimiter than colon, since users might reasonably search for "x: y" and get a false/confusing positive here. (In theory, they can search for a newline too, I guess, but not reasonably using the normal UI).

This revision now requires changes to proceed.Feb 3 2017, 1:55 PM

Looks good. Couple of minor nits inline.

Your test plan should also include something like this:

  • Edited a repository to have "platypus" in the description, searched for "platypus", found the repository.

Even when bin/search index works, the actual "edit stuff with the web UI" workflow may still not be fully hooked up. Usually, this is because the Editor does not override supportsSearch(), so edits don't rebuild the index. In this case it looks like you're in the clear, so I'd expect this to work correctly with no further changes, but it's worth double checking.

Ok I'll test re-indexing, I think that works fine but you're right that I should have tested more thoroughly and mentioned it in my test plan. ;)

Also, thanks for pointing out "supportsSearch" I wasn't aware of the need to override that method.

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
10

I was trying to get code intelligence to act intelligently. It didn't work and I forgot to delete the comment.

Code intelligence: still not very intelligent in 2017.

15

Good point.

20after4 edited edge metadata.
20after4 marked 2 inline comments as done.
20after4 edited the test plan for this revision. (Show Details)

Addressed epriestley's feedback.

resubmit with arc diff --config repository.callsign=P

For some reason, arc can't determine the repo and skips staging unless I
explicitly specify the callsign.

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
13

I guess I could actually add the field twice instead of concatenating with a \n, at least that works fine with elasticsearch. I wonder how it would work with mysql...

In terms of figuring out which repository your working copy is, what does arc which show?

One minor inline.

(I think the MySQL schema makes the field key (body, etc) unique, so you can't add multiple fields of the same type.)

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
14

'\n' in single quotes isn't a newline in PHP. 🐈

epriestley@orbital ~ $ cat test.php 
<?php

echo "With Double Quotes: <\n>";
echo "\n";
echo 'With Single Quotes: <\n>';
echo "\n";
epriestley@orbital ~ $ php -f test.php 
With Double Quotes: <
>
With Single Quotes: <\n>
This revision is now accepted and ready to land.Mar 26 2017, 12:11 PM
src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
14

doh!

thatwasobvious

surprised arc doesn't have a lint rule for '\n' ...but I guess there are legitimate uses for it occasionally.

It probably should -- you could always write \\n if you really wanted a literal backslash + n -- I think we just haven't hit it before.

(Also, our parsing of the grammar inside string literals isn't the greatest: T11081, etc.)

push to staging for harbormaster

This revision was automatically updated to reflect the committed changes.