1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-18 04:42:40 +01:00
Commit graph

17376 commits

Author SHA1 Message Date
epriestley
f45a13cff4 Improve settings caches on fast paths like Conduit
Summary:
Ref T11954. This reduces how much work we need to do to load settings, particularly for Conduit (which currently can not benefit directly from the user cache, because it loads the user indirectly via a token).

Specifically:

  - Cache builtin defaults in the runtime cache. This means Phabricator may need to be restarted if you change a global setting default, but this is exceptionally rare.
  - Cache global defaults in the mutable cache. This means we do less work to load them.
  - Avoid loading settings classes if we don't have to.
  - If we missed the user cache for settings, try to read it from the cache table before we actually go regenerate it (we miss on Conduit pathways).

Test Plan: Used `ab -n100 ...` to observe a ~6-10ms performance improvement for `user.whoami`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11954

Differential Revision: https://secure.phabricator.com/D16998
2016-12-06 09:12:10 -08:00
epriestley
125fb332de Introduce a serializing key-value cache proxy
Summary:
Ref T11954. I want to store some lists/arrays in the mutable (database) cache, but it only supports string storage.

Provide a serializing wrapper which flattens when values are written and expands them when they're read.

Test Plan: Used by D16997. See that revision.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11954

Differential Revision: https://secure.phabricator.com/D16999
2016-12-06 09:11:32 -08:00
epriestley
f8d6b6181e Use PhabricatorCachedClassMapQuery when querying object PHID types
Summary:
Ref T11954. When we query for Conduit tokens, we load the associated objects (users) by PHID.

Currently, querying objects by PHID requires us to load every PHIDType class, when we can know which specific classes we actually need (e.g., just `UserPHIDType`, if only user PHIDs are present in the query).

Use PhabricatorCachedClassMapQuery to reduce the number of classes we load on this pathway.

Test Plan:
- Used `ab -n100` to roughly measure a ~5% performance improvement?
- This measurement feels a little flimsy but the XHProf profile is cleaner, at least.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11954

Differential Revision: https://secure.phabricator.com/D16997
2016-12-06 09:10:29 -08:00
epriestley
bfbf75a872 Slightly modernize ConduitTokenQuery
Summary: Ref T11954. This old query class can use slightly more modern code.

Test Plan: Ran Conduit methods, verified results are unchanged.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11954

Differential Revision: https://secure.phabricator.com/D16996
2016-12-06 08:45:43 -08:00
epriestley
55a54facd5 Use PhabricatorCachedClassMapQuery in Conduit method lookups
Summary: Ref T11954. Depends on D16994. This implements the Conduit method cache described in that revision for a small global Conduit performance improvement.

Test Plan: Verified Conduit has the same behavior at lower cost. See D16994 for details.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11954

Differential Revision: https://secure.phabricator.com/D16995
2016-12-06 08:38:46 -08:00
epriestley
1f3fcce6fe Provide a cached class map query for making key-based class lookups more efficient
Summary:
Ref T11954. Depends on D16993. We have a couple of "look up the class for this key" queries which are costly enough to show up on a profile.

These aren't huge wins, but they're pretty easy. We currently do this like this:

```
$class_map = load_every_subclass();
return idx($class_map, $key);
```

However, we don't need to load EVERY subclass if we're only looking for, say, the Conduit method subclass which implements `user.whoami`. This allows us to cache that map and find the right class efficiently.

This cache is self-validating and completely safe even in development.

Test Plan:
  - Used `curl` to make queries to `user.whoami`, verified that content was identical before and after the change.
  - Used `ab -n100` to roughly measure 99th percentile time, which dropped from 74ms to 65ms. This is a small improvement (13% in the best case, here) but it benefits every Conduit method call.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11954

Differential Revision: https://secure.phabricator.com/D16994
2016-12-06 08:34:29 -08:00
epriestley
52112620a3 Provide a pure APC cache for runtime caching
Summary:
Ref T11954. Depends on D16992. We have some data which can be generated and cached at runtime. Three examples are:

  - Class map from Conduit method names to implementing classes.
  - Class map from PHID types to implementing classes.
  - The main routing map.

None of these are huge wins but they impose global costs and can be shaved down through caching without introducing an enormous amount of new complexity.

The cost to these maps is that sometimes you'll need to restart your webserver, even in development mode if these caches are active. However, in some cases these changes are very rare, and in other cases we can just leave the cache disabled in development mode without a huge complexity cost.

Specifically, the Conduit/PHID type class maps are self-validating and can not go bad, even in development mode.

The routing map will be able to, but I plan to just disable it in development mode.

This provides a general-purpose pure APC cache stack for storing this data.

Test Plan: See future changes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11954

Differential Revision: https://secure.phabricator.com/D16993
2016-12-06 08:34:13 -08:00
epriestley
4faa4b451f When viewing a branch, preview differences from master
Summary: Ref T929. When viewing a branch, show a few recent differences from the default branch (usually, "master").

Test Plan: {F2079220}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T929

Differential Revision: https://secure.phabricator.com/D16991
2016-12-06 08:16:41 -08:00
epriestley
fc1adf9875 Modernize UI for "Compare" in Diffusion
Summary: Ref T929. We've made some UI updates since D15330.

Test Plan: {F2079125}

Reviewers: avivey, chad

Reviewed By: chad

Maniphest Tasks: T929

Differential Revision: https://secure.phabricator.com/D16990
2016-12-05 18:10:11 -08:00
Aviv Eyal
43f9927a38 Compare two branches
Summary:
This shows the commits list only (Actual `git diff` will show up at a later date).
The inputs are left as text-fields, to allow the form to accept anything that can be resolved. The form is GET, to allow sharing URIs.

The conduit method response array is compatible with that of `diffusion.historyquery`, to make it easy to build
the "history" table.

The hardest part here was, of course, Naming. I think "from" and "onto" are unconfusing, and I'm fairly confident that the "to merge"
instructions are in sync with the actual content of the page.

Test Plan: Look at several "compare" views, with various values of "from" and "onto".

Reviewers: #blessed_reviewers!, epriestley

Subscribers: caov297, 20after4, Sam2304, reardencode, baileyb, chad, Korvin

Maniphest Tasks: T929

Differential Revision: https://secure.phabricator.com/D15330
2016-12-05 16:25:49 -08:00
Eitan Adler
0ad1dd640a Remove the Persona login method
Summary:
Persona is going to be decommed November 30th, 2016.
It is highly unlikely that anyone is currently using persona as a real
login method at this point.

Test Plan: tried locally to add auth adapter.

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D16371
2016-12-05 15:57:15 -08:00
epriestley
005d8493b0 Pass GIT_ENVIRONMENTAL_MAGIC through to hook subprocesses to support Git 2.11.0
Summary:
Fixes T11940. In 2.11.0, Git has made a change so that newly-pushed changes are held in a temporary area until the hook accepts or rejects them.

This magic temporary area is only readable if the appropriate `GIT_ENVIRONMENTAL_MAGIC` variables are available. When executing `git` commands, pass them through from the calling context.

We're intentionally conservative about which variables we pass, and with good reason (see "httpoxy" in T11359). I think this continues to be the correct default behavior.

Test Plan:
  - Upgraded to Git 2.11.0.
  - Tried to push over SSH, got a hook error.
  - Applied patch.
  - Pulled and pushed over SSH.
  - Pulled and pushed over HTTP.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11940

Differential Revision: https://secure.phabricator.com/D16988
2016-12-05 12:45:30 -08:00
epriestley
6058d3305f Normalize remote IP addresses when writing to logs, etc
Summary:
Ref T11939. IPv4 addresses can normally only be written in one way, but IPv6 addresses have several formats.

For example, the addresses "FFF::", "FfF::", "fff::", "0ffF::", "0fFf:0::", and "0FfF:0:0:0:0:0:0:0" are all the same address.

Normalize all addresses before writing them to logs, etc, so we store the most-preferred form ("fff::", above).

Test Plan:
Ran an SSH clone over IPv6:

```
$ git fetch ssh://local@::1/diffusion/26/locktopia.git
```

It worked; verified that address read out of `SSH_CLIENT` sensibly.

Faked my remote address as a non-preferred-form IPv6 address using `preamble.php`.

Failed to login, verified that the preferred-form version of the address appeared in the user activity log.

Made IPv6 requests over HTTP:

```
$ curl -H "Host: local.phacility.com" "http://[::1]/"
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11939

Differential Revision: https://secure.phabricator.com/D16987
2016-12-05 11:20:29 -08:00
epriestley
5a060b34df Add IPv6 reserved addresses to the default outbound blacklist
Summary:
Ref T11939. Depends on D16984. Now that CIDRLists can contain IPv6 addresses, blacklist all of the reserved IPv6 space.

This reserved blacklist is used to prevent users from accessing internal services via "Import Calendar" or "Add Macro".

They can't actually reach IPv6 addresses via these mechanisms yet because we need to do more work to support outbound IPv6 requests, but make sure reserved IPv6 space is blacklisted already when that support eventaully arrives.

Also, clean up some error messages (e.g., for trying to hit a bad URI in "Add Macro").

Test Plan:
  - Loaded pages with default blacklist.
  - Tried to make requests into IPv6 space.
  - Currently, this is impossible because of `parse_url()` and `gethostynamel()` calls.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11939

Differential Revision: https://secure.phabricator.com/D16986
2016-12-05 11:20:13 -08:00
epriestley
4a6229ee69 Remove some no-op "canUninstall()" Application methods
Summary: The default behavior of these methods is to return `true`, so these overrides have no effect.

Test Plan: `grep`; poked around.

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Differential Revision: https://secure.phabricator.com/D16985
2016-12-05 11:02:25 -08:00
epriestley
5f593aafb1 Allow logged-out users to load global preferences on installs without public viewers
Summary:
Fixes T11946. When a logged-out viewer is loading a page on a non-public install, there are two policy issues which prevent them from loading global settings:

  - They can not see the Settings application itself.
  - They can not see the global settings object.

Allow them to see Settings by making mandatory applications always visible. (This doesn't make any application pages public.)

Allow them to see the global settings object explicitly.

Test Plan:
Changed default language, viewed logged-out page:

{F2076924}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11946

Differential Revision: https://secure.phabricator.com/D16983
2016-12-05 11:00:39 -08:00
Chad Little
f0bf0419f1 Hide badges in phui-comment-panel
Summary: Ref T3612. Hides badges on the comment panel.

Test Plan: Give myself a badge, leave a comment, see no badge UI.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T3612

Differential Revision: https://secure.phabricator.com/D16979
2016-12-02 11:02:53 -08:00
Chad Little
49a20bde8e Prevent double lightbox in lightbox comments
Summary: Ref T3612, prevents lightbox from spawning from inside a lightbox.

Test Plan: Click on file lightbox, leave file comment, click file comment, get take to file page instead of another lightbox.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T3612

Differential Revision: https://secure.phabricator.com/D16978
2016-12-02 10:56:21 -08:00
Chad Little
aa6517a42b Make clicking on file icon in lightbox download file
Summary: Ref T3612, this adds a anchor around the large icon with hover state so you can download from here as well.

Test Plan: Hover over .ics file, click, get download.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T3612

Differential Revision: https://secure.phabricator.com/D16977
2016-12-02 10:21:28 -08:00
epriestley
faf983614c Improve error messages for running git clone against a Mercurial repository
Summary:
Fixes T11938.

Note that there's a subcase here: if you `hg clone` or `svn checkout` a short `/source/` URI that ends in `.git`, we miss the lookup and don't get this far, so you still get a generic error message.

Hopefully it is clear enough on its own that `proto://.../blah.git` is, in fact, a Git repository, since it says ".git" at the end.

If that doesn't prove to be true, we can be more surgical about this.

Test Plan:
```
$ git clone ssh://local@localvault.phacility.com/source/quack.notgit/
Cloning into 'quack.notgit'...
phabricator-ssh-exec: This repository ("quack.notgit") is not a Git repository. Use "hg" to interact with this repository.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
```

```
$ hg clone ssh://local@localvault.phacility.com/source/phabx
remote: phabricator-ssh-exec: This repository ("phabx") is not a Mercurial repository. Use "git" to interact with this repository.
abort: no suitable response from remote hg!
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11938

Differential Revision: https://secure.phabricator.com/D16976
2016-12-02 07:30:03 -08:00
epriestley
7c37377e0d Set the viewer timezone properly on Calendar event RecurrenceSet objects
Summary: Ref T11801. In some cases, this could lead to us failing to generate the first recurrence in a series.

Test Plan: Imported `weekly.ics` (from D16974) and saw an event correctly occur on Aug 18, with my local timezone set to "America/Los_Angeles".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11801

Differential Revision: https://secure.phabricator.com/D16975
2016-12-02 07:29:48 -08:00
epriestley
99c6b53ab2 Explicitly update the repository URI index after making a URI edit
Summary:
Fixes T11936. After editing a repository URI, we were not correctly updating the URI index.

Any other edit to the repository //would// update the index, and this index is only really used by `arc` to figure out which repository a working copy belongs to, so that's how this evaded detection for this long. In particular, creating a repository would usually have an edit after any URI edits, to activate it, which would build the index correctly.

Test Plan:
  - Added a new URI to a repository.
  - Verified it was immediately reflected in the `repository_uriindex` table.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11936

Differential Revision: https://secure.phabricator.com/D16972
2016-12-01 14:29:39 -08:00
epriestley
dc73785c4f Add a "--force" argument to "bin/config done"
Summary:
Ref T11922. When we deploy on Saturday I need to rebuild all the cluster indexes, but some instances won't have anything indexed so they won't actually trigger the activity.

Add a `--force` flag that just clears an activity even if the activity is not required.

Test Plan: Ran `bin/config done reindex --force` several times.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11922

Differential Revision: https://secure.phabricator.com/D16970
2016-12-01 13:53:33 -08:00
Chad Little
0db28cfc01 Wipe textarea immediately when submitting a new message in Conpherence
Summary: Fixes T11791. We do this in durable column, but not in regular Conpherence. I think this is the right place? Not sure how this will feel with high lag.

Test Plan: Submit lots of text in a Conpherence.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11791

Differential Revision: https://secure.phabricator.com/D16969
2016-12-01 10:08:11 -08:00
Chad Little
98057091f6 Remove default margin on selects
Summary: Fixes T11935. Safari even with appearance: none still assigns margin to selects. Reset margin to 0.

Test Plan: Review selects on Safari, Chrome.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11935

Differential Revision: https://secure.phabricator.com/D16968
2016-12-01 09:08:40 -08:00
epriestley
9730f5a34f Allow custom Sites to have custom 404 controllers
Summary:
Currently, custom Sites must match `.*` or similar to handle 404's, since the fallback is always generic.

This locks them out of the "redirect to canonicalize to `path/` code", so they currently have a choice between a custom 404 page or automatic correction of `/`.

Instead, allow the 404 controller to be constructed explicitly. Sites can now customize 404 by implementing this method and not matching everything.

(Sites can still match everything with a catchall rule if they don't want this behavior for some reason, so this should be strictly more powerful than the old behavior.)

See next diff for CORGI.

Test Plan:
  - Visited real 404 (like "/asdfafewfq"), missing-slash-404 (like "/maniphest") and real page (like "/maniphest/") URIs on blog, main, and CORGI sites.
  - Got 404 behavior, redirects, and real pages, respectively.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16966
2016-11-30 15:25:09 -08:00
epriestley
29a3cd5121 Add "Manual Activities", to tell administrators to rebuild the search index
Summary:
Ref T11922. After updating to HEAD of `master`, you need to manually rebuild the index. We don't do this during `bin/storage upgrade` because it can take a very long time (`secure.phabricator.com` took roughly an hour) and can happen while Phabricator is running.

However, if we don't warn users about this they'll just get a broken index unless they go read the changelog (or file an issue, then we tell them to go read the changelog).

This adds a very simple table for notes to administrators so we can write a "you need to go rebuild the index" note, then adds one.

Administrators clear the note by completing the activity and running `bin/config done reindex`. This isn't automatic because there are various strategies you can use to approach the issue, which I'll discuss in greater detail in the linked documentation.

Also, fix an issue where `bin/storage upgrade --apply <patch>` could try to re-mark an already-applied patch as applied.

Test Plan:
  - Ran storage ugrades.
  - Got instructions to rebuild search index.
  - Cleared instructions with `bin/config done reindex`.

Reviewers: chad

Reviewed By: chad

Subscribers: avivey

Maniphest Tasks: T11922

Differential Revision: https://secure.phabricator.com/D16965
2016-11-30 11:23:54 -08:00
Chad Little
eeb80ba96b Add sidenav back to workboards
Summary: This is still reasonably functional and useful to people, and we don't have better mechanics to offset the change.

Test Plan: New Workboard, set Workboard color, test mobile, desktop.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16964
2016-11-30 09:56:55 -08:00
Chad Little
f844280d36 Make lightbox buttons more roundy
Summary: Ref T3612. Mobilizes the new lightbox, changes large buttons to circle icons like Conpherence.

Test Plan: Click each new button on desktop, mobile.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T3612

Differential Revision: https://secure.phabricator.com/D16961
2016-11-29 12:23:05 -08:00
epriestley
23a202866a When running a fulltext query with no query, enforce order by document creation date
Summary:
Fixes T11929. When running with a query, we no longer enforce an order on the subquery join to produce results more quickly when searching for common strings.

However, this means that empty queries (like those issued by "Close as Duplicate") don't order subquery results.

Restore a `dateCreated` order if there is no query text.

Test Plan: Artificially set limit to 10, still saw 10 most recent tasks.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11929

Differential Revision: https://secure.phabricator.com/D16960
2016-11-29 12:14:57 -08:00
epriestley
2d7abfd9fa Use HTTPS, not HTTP, in install scripts
Summary:
Via HackerOne. A researcher correctly reports that our install scripts use `HTTP`, not `HTTPS`, to fetch resources and execute them as `root`, which is a potentially significant vulnerability.

Instead, use `HTTPS`.

Test Plan: Verified that these URIs function correctly over `HTTPS`.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16958
2016-11-29 12:11:04 -08:00
Chad Little
d1838fa5ec Close lightbox when clicking on image-frame
Summary: Ref T3612. Moves the listener to the frame of the image.

Test Plan: Click on image, no close, click on grey frame, closes image. Test image and document, clicking on arrows.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T3612

Differential Revision: https://secure.phabricator.com/D16959
2016-11-29 18:35:04 +00:00
Chad Little
80570d668b Use proper file icons in lightbox
Summary: Ref T3612. Passes in file size and file icon for non-images.

Test Plan: Review a PDF and PSD in a lightbox.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T3612

Differential Revision: https://secure.phabricator.com/D16957
2016-11-28 11:30:19 -08:00
epriestley
5cc4f6407c Make the modal choice between "Edit This Event" and "Edit Future Events" more clear
Summary:
Fixes T11909. Ref T11816. Instead of offering a dropdown with choices between "Edit/Cancel/Reinstate This Event" and "Edit/Cancel/Reinstate Future Events", make the choice more explicit.

This dialog ends up pretty wordy but this edit is rare, so I think that's alright.

Test Plan: {F2046863}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11816, T11909

Differential Revision: https://secure.phabricator.com/D16956
2016-11-28 10:47:58 -08:00
epriestley
22a566f732 Ignore Calendar date edits which just change the internal date timezone without rescheduling it
Summary:
Ref T11816. Currently, if someone in California creates an event and then someone in New York edits it, we generate a no-op "<user> changed the start time from 3PM to 3PM." transaction.

This is because the internal timezone of the event is changing, but the actual absolute time is not.

Instead, when an edit wouldn't reschedule an event and would only change the internal timezone, ignore the edit.

Test Plan:
  - Edited non-all-day events in PST / EST with out making changes (ignored).
  - Edited non-all-day events in PST / EST with changes (changes worked).
  - Performed the same edits with all-day events, which also were ignored and worked, respectively.
  - Pulled events in and out of all-day mode in different timezones, behavior seemeed reasonable.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11816

Differential Revision: https://secure.phabricator.com/D16955
2016-11-28 10:33:59 -08:00
epriestley
ab3b707396 Fix local time test case for logged-out viewers using global settings
Summary:
In D16936, I changed logged-out viewers so they use global settings.

This can lead to a `SELECT` from an isolated unit test. Instead, give the test fixtures and use standard `generateNewUser()` stuff.

Test Plan: Ran `arc unit --everything`.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16952
2016-11-28 09:27:11 -08:00
epriestley
c5162074a1 Fix an issue where internal Calendar DateTimes would not be correctly set to all-day
Summary:
Ref T11816. I don't really know what happened here, maybe I rewrote and broke this at the last second?

In most cases, we directly respect the `isAllDay` flag on the event, so the internal date state doesn't matter too much.

However, in the case of mail notifications, the raw internal state is relevant. This should fix mail notifications for all-day events.

(I might still turn them off since I'm not sure they're too useful, but it's good to have them working.)

Test Plan:
  - Created a new all-day event, verified database values wrote correctly.
  - Ran `bin/calendar notify --trace`, verified it picked up an all-day event tomorrow with a large enough `--minutes` value.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11816

Differential Revision: https://secure.phabricator.com/D16954
2016-11-28 08:54:28 -08:00
Chad Little
3edd01bc0f Remove english from lightbox javascript
Summary: Reduces to x / y for lightbox. Adds spacing. Ref T3612

Test Plan: Review spacing in sandbox.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T3612

Differential Revision: https://secure.phabricator.com/D16951
2016-11-28 08:48:14 -08:00
Chad Little
dece7af50b Prettier file embeds
Summary: Spruce up the file embeds a little more, hover state, icons, file size.

Test Plan:
Add a psd and pdf, see new icons. Check differential, still see icons there too. Test mobile, desktop.

{F2042539}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D16950
2016-11-27 14:57:06 -08:00
epriestley
b2cdebefea Fix two errors from the error logs
Summary: Found these in the `secure` error logs: one bad call, one bad column.

Test Plan: Searched for empty string. Double-checked method name.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D16948
2016-11-26 07:50:57 -08:00
epriestley
7c5b5327c8 Use stemming in the MySQL fulltext search engine
Summary:
Ref T6740. When we index a document, also save a copy of the stemmed version.

When querying, search the combined corpus for the terms.

(We may need to tune this a bit later since it's possible for literal, quoted terms to match in the stemmed section, but I think this wil rarely cause issues in practice.)

A downside here is that search sort of breaks if you upgrade into this and don't reindex. I wasn't able to find a way to issue the query that remained compatible with older indexes and didn't have awful performance, so my plan is:

  - Put this on `secure`.
  - Rebuild the index.
  - If things look good after a couple of days, add a way that we can tell people they need to rebuild the search index with a setup warning.

We might get some reports between now and then, but if this is super awful we should know by the end of the weekend.

Test Plan:
WOW AMAZING

{F2021466}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6740

Differential Revision: https://secure.phabricator.com/D16947
2016-11-25 15:30:50 -08:00
epriestley
d54c14c644 If InnoDB FULLTEXT is available, use it for for fulltext indexes
Summary: Ref T11741. I'll wait until the release cut to land this; it just adds a test for InnoDB FULLTEXT being available instead of always returning `false`.

Test Plan:
  - Ran with InnoDB fulltext locally for a day and a half without issues.
  - Ran `bin/storage upgrade`, saw it detect InnoDB fulltext.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11741

Differential Revision: https://secure.phabricator.com/D16946
2016-11-25 15:29:14 -08:00
epriestley
2b7ec1deea Boost search result title matches
Summary: Ref T6740. When a query matches a document title, boost results.

Test Plan:
  - Searched for `button bar`.
  - Before:

{F2019463}

  - After:

{F2019470}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6740

Differential Revision: https://secure.phabricator.com/D16945
2016-11-25 15:25:49 -08:00
epriestley
54470a12d4 Execute fulltext queries using a subquery instead of by ordering the entire result set
Summary:
Ref T6740. Currently, we issue fulltext queries with an "ORDER BY <score>" on the entire result set.

For very large result sets, this can require MySQL to do a lot of work. However, this work is generally useless: if you search for some common word like "diff" or "internet" or whatever and match 4,000 documents, the chance that we can score whatever thing you were thinking of at the top of the result set is nearly nothing. It's more useful to return quickly, and let the user see that they need to narrow their query to get useful results.

Instead of doing all that work, let MySQL find up to 1,000 results, then pick the best ones out of those.

This actual change is a little flimsy, since our index isn't really big enough to suffer indexing issues. However, searching for common terms on my local install (where I have some large repositories imported and indexed) drops from ~40ms to ~10ms.

My hope is to improve downstream performance for queries like "translatewiki" here, particularly:

<https://phabricator.wikimedia.org/T143863>

That query matches about 300 trillion documents but there's a ~0% chance that the one the user wants is at the top. It takes a couple of seconds to execute, for me. Better to return quickly and let the user refine their results.

I think this will also make some other changes related to stemming easier.

This also removes the "list users first" ordering on the query, which made performance more complicated and seems irrelevant now that we have the typeahead.

Test Plan:
  - Searched for some common terms like "code" locally, saw similar results with better performance.
  - Searched for useful queries (e.g., small result set), got identical results.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6740

Differential Revision: https://secure.phabricator.com/D16944
2016-11-25 15:19:54 -08:00
epriestley
48a34eced2 Prepare for InnoDB FULLTEXT support
Summary:
Ref T11741. This makes everything work if we switch to InnoDB, but never actually switches yet.

Since the default minimum word length (3) and stopword list (36 common English words) in InnoDB are generally pretty reasonable, I just didn't add any setup advice for them. I figure we're better off with simpler setup until we identify some real problem that the builtin stopwords create.

Test Plan: Swapped the `false` to `true`, ran `storage adjust`, got InnoDB fulltext indexes, searched for stuff, got default "AND" behavior.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11741

Differential Revision: https://secure.phabricator.com/D16942
2016-11-25 15:18:26 -08:00
epriestley
9d0752063e Allow bin/storage adjust to adjust table engines
Summary:
Ref T11741. On recent-enough versions of MySQL, we would prefer to use InnoDB for fulltext indexes instead of MyISAM.

Allow `bin/storage adjust` to read actual and expected table engines, and apply adjustments as necessary.

We have one existing bad table that uses the wrong engine, `metamta_applicationemail`. This change corrects that table.

Test Plan:
  - Ran `bin/storage upgrade`.
  - Saw the adjustment phase apply this change properly:

```
>>>[463] <query> ALTER TABLE `local_metamta`.`metamta_applicationemail` COLLATE = 'utf8mb4_bin', ENGINE = 'InnoDB'
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11741

Differential Revision: https://secure.phabricator.com/D16941
2016-11-25 15:13:40 -08:00
epriestley
ff3333548f Create and populate a stopwords table for InnoDB fulltext indexes to use in the future
Summary:
Ref T11741. InnoDB uses a stopwords table instead of a stopwords file.

During `storage upgrade`, synchronize the table from the stopwords file on disk.

Test Plan:
  - Ran `storage upgrade`.
  - Ran `select * from stopwords`, saw stopwords.
  - Added some garbage to the table.
  - Ran `storage upgrade`, saw it remove it.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11741

Differential Revision: https://secure.phabricator.com/D16940
2016-11-25 15:13:08 -08:00
epriestley
a956047989 Use PhutilQueryCompiler in Phabricator fulltext search
Summary:
Ref T11741. Fixes T10642. Parse and compile user queries with a consistent ruleset, then submit queries to the backend using whatever ruleset MySQL is configured with.

This means that `ft_boolean_syntax` no longer needs to be configured (we'll just do the right thing in all cases).

This should improve behavior with RDS immediately (T10642), and allow us to improve behavior with InnoDB in the future (T11741).

Test Plan:
  - Ran various queries in the UI, saw the expected results.
  - Ran bad queries, got useful errors.
  - Searched threads in Conpherence.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10642, T11741

Differential Revision: https://secure.phabricator.com/D16939
2016-11-25 14:46:10 -08:00
epriestley
eac49e421a Fix an issue where an excessively long TTL was computed for "not attending anything" event caches
Summary:
Fixes T11894. Currently, if you aren't attending any events for a while, we can cache that you are free for the next 72 hours, even if you have an event in a few hours.

Instead, only cache "user is free" until the next event, if one exists.

Test Plan: Dumped cache TTLs, saw 52 minutes instead of ~4300 minutes with a near-upcoming event.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11894

Differential Revision: https://secure.phabricator.com/D16937
2016-11-23 15:22:13 -08:00
epriestley
a1025ca52e Make logged-out users use global settings, not default settings
Summary: Fixes T11917. Give logged-out / omnipotent users the global settings, not the default settings.

Test Plan: Changed applications and language, logged out, saw changes as a public user.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11917

Differential Revision: https://secure.phabricator.com/D16936
2016-11-23 14:26:00 -08:00