Add repositories to fulltext search index.
ClosedPublic

Authored by 20after4 on Feb 2 2017, 6:34 PM.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
20after4 created this revision.Feb 2 2017, 6:34 PM
epriestley requested changes to this revision.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.

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
20after4 marked 2 inline comments as done.Feb 18 2017, 8:40 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 the test plan for this revision. (Show Details)Sun, Mar 26, 8:28 AM
20after4 updated this revision to Diff 42245.Sun, Mar 26, 8:42 AM
20after4 marked 2 inline comments as done.
20after4 edited the test plan for this revision. (Show Details)

Addressed epriestley's feedback.

20after4 updated this revision to Diff 42246.Sun, Mar 26, 8:44 AM

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.

20after4 added inline comments.Sun, Mar 26, 11:53 AM
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?

epriestley accepted this revision.Sun, Mar 26, 12:11 PM

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.Sun, Mar 26, 12:11 PM
20after4 added inline comments.Sun, Mar 26, 12:22 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.)

20after4 updated this revision to Diff 42261.Tue, Mar 28, 7:56 AM

push to staging for harbormaster

20after4 marked 2 inline comments as done.Tue, Mar 28, 7:57 AM
This revision was automatically updated to reflect the committed changes.