1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-22 13:30:55 +01:00
Commit graph

16907 commits

Author SHA1 Message Date
epriestley
527dd3ce50 Replace Differential "unit stars" with icons
Summary:
Ref T9764. These "star" icons are unclear, inconsistent, and not friendly to colorblind users.

They date from a time long ago when the product didn't have icons.

Modernize them and make them more consistent with the similar statuses in Harbormaster.

Test Plan:
{F8545690}

{F8545691}

{F8545692}

Maniphest Tasks: T9764

Differential Revision: https://secure.phabricator.com/D21639
2021-03-17 13:45:52 -07:00
epriestley
1d1003af78 When using "Update Diff" from the web UI, prefill "Repository" properly
Summary: Ref T9499. When using the manual "Update Diff" workflow on the web, the "Repository" field isn't pre-filled properly. This can lead to revisions losing their repository after a manual update.

Test Plan: Did a manual update of a revision with a repository, saw it stick.

Maniphest Tasks: T9499

Differential Revision: https://secure.phabricator.com/D21638
2021-03-17 12:23:24 -07:00
epriestley
ff0a4a2c6f Use a less-misleading example for Conduit custom constraints
Summary:
See PHI2032. The Conduit UI shows some "Example Custom Constraints" that are intended to be generic, but use of "statuses" is misleading since many objects have a status and most of them don't support these specific values.

Make it more clear that these are generic values.

Test Plan: Read new text.

Differential Revision: https://secure.phabricator.com/D21637
2021-03-17 11:47:41 -07:00
epriestley
e6a23274fe Default the Almanac Devices query to "Active Devices"
Summary: Ref T13641. Now that we can provide an "Active Devices" query, provide it and make it the default.

Test Plan: Viewed Almanac devices, got a list of active devices. Clicked "All Devices" to get all devices.

Maniphest Tasks: T13641

Differential Revision: https://secure.phabricator.com/D21636
2021-03-16 15:51:51 -07:00
epriestley
12341e4bc8 Forbid disabled devices from authenticating via SSH or HTTP
Summary:
Ref T13641. Phabricator sometimes makes intracluster requests that authenticate as a device.

Forbid these requests from authenticating as a disabled device.

Test Plan:
  - Ran `bin/ssh-exec --phabricator-ssh-device ...` as an enabled/disabled device (worked; sensible error).
  - Made Conduit calls as an enable/disabled device (worked; sensible error).

Maniphest Tasks: T13641

Differential Revision: https://secure.phabricator.com/D21635
2021-03-16 15:51:51 -07:00
epriestley
3267859aee Modernize "mailKey" on Fund initiatives
Summary: Ref T13065. Migrate "mailKey" and drop the column.

Test Plan: Ran "bin/storage upgrade", got a clean upgrade, saw migrated values in mail properties table.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13065

Differential Revision: https://secure.phabricator.com/D21634
2021-03-16 15:51:50 -07:00
epriestley
eeb009b4fa Modernize "mailKey" for Calendar Event
Summary: Ref T13065. Migrate "mailKey" and drop the column.

Test Plan: Ran "bin/storage upgrade", got a clean report, saw migrated data in mail properties table.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13065

Differential Revision: https://secure.phabricator.com/D21633
2021-03-16 15:51:50 -07:00
epriestley
86669ab54f Modernize "mailKey" for Almanac Networks
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.

Test Plan: Ran "bin/storage upgrade", got a clean report, saw keys migrate in mail properties table.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13641, T13065

Differential Revision: https://secure.phabricator.com/D21632
2021-03-16 15:51:49 -07:00
epriestley
8226b3c880 Modernize "mailKey" on Almanac Namespaces
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.

Test Plan: Ran "bin/storage upgrade", got a clean report, saw mail keys migrated to mail properties table.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13641, T13065

Differential Revision: https://secure.phabricator.com/D21631
2021-03-16 15:51:49 -07:00
epriestley
7c0e33c34d Modernize "mailKey" for Almanac Bindings
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.

Test Plan: Ran "bin/storage upgrade", got a clean report and saw binding mail keys in the mail properties table.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13641, T13065

Differential Revision: https://secure.phabricator.com/D21630
2021-03-16 15:51:48 -07:00
epriestley
fadb2bd52b Modernize "mailKey" on AlamnacService
Summary: Ref T13641. Ref T13065. Migrate and drop the onboard "mailKey" column for Almanac Services.

Test Plan: Ran storage migration, got clean report, saw migrated value in mail properties table.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13641, T13065

Differential Revision: https://secure.phabricator.com/D21629
2021-03-16 15:51:48 -07:00
epriestley
15c0c895a5 Make upstream callers respect "active bindings" when querying Almanac
Summary: Ref T13641. Make "active bindings" a real query and make callers that only care about active bindings only query for active bindings.

Test Plan:
  - Queried for "bindings" and "activeBindings" via Conduit.
  - Disabled/enabled devices, saw binding status update in UI.
  - Loaded Diffusion cluster layout.
  - Grepped for `needBindings()`, `getActiveBindings()`, etc.

Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13641

Differential Revision: https://secure.phabricator.com/D21628
2021-03-16 15:51:47 -07:00
epriestley
5d64fb1815 Add a "status" property to Almanac devices
Summary:
Ref T13641. Add a "status" property with most of the relevant support code.

This currently has no impact on use of the device or bindings by Diffusion or Drydock: they ignore the status of devices bound to services.

Test Plan:
  - Created a new device.
  - Changed the status of a device via web and API.
  - Queried for devices via API.
  - Searched for active and disabled devices.
  - Viewed UI in list view, detail view.
  - Used typeahead to add a new binding to an interface on a disabled device, got disabled hint in typeahead UI.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13641

Differential Revision: https://secure.phabricator.com/D21627
2021-03-16 15:51:47 -07:00
epriestley
9003a45369 Make minor Almanac device modernization updates
Summary: Ref T13641. Provide minor modernizations before adding a "Disabled" state.

Test Plan: Browsed devices, created a new device.

Maniphest Tasks: T13641

Differential Revision: https://secure.phabricator.com/D21626
2021-03-16 15:51:46 -07:00
epriestley
c3e6db6f0b Migrate Almanac Device "mailKey" to modern storage
Summary: Ref T13065. See similar changes attached to that task.

Test Plan: Ran migration, got a clean database state, saw mail keys populate in mail property table.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13065, T13641

Differential Revision: https://secure.phabricator.com/D21625
2021-03-16 15:51:46 -07:00
epriestley
42c0c0e3d2 Remove or correct various "phabricator/" references to "libphutil"
Summary:
Ref T13395. "libphutil/" was stripped for parts, but some documentation still references it. This is mostly minor corrections, but:

  - Removes "Javelin at Facebook", long obsolete.
  - Removes "php FPM warmup", which was always a prototype and is obsoleted by PHP preloading in recent PHP.

Test Plan: `grep` / reading

Maniphest Tasks: T13395

Differential Revision: https://secure.phabricator.com/D21624
2021-03-16 10:28:07 -07:00
epriestley
30d58de1bc Expose "affectedPaths" to "differential.revision.search" Conduit API method
Summary: Ref T13639. Support querying by "affectedPaths" in the API.

Test Plan: {F8539347}

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21621
2021-03-15 16:16:11 -07:00
epriestley
6d33ba7dc4 Move Diffusion to "withPaths()" for "Recent Open Revisions", and remove "withPath()" from DifferentialRevisionQuery
Summary: Ref T13639. Move Diffusion to use the new API and get rid of the old API now that it no longer has any callers.

Test Plan:
Grepped for remaining callers.

{F8539335}

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21620
2021-03-15 16:16:11 -07:00
epriestley
925c9a71e7 Support a "withPaths()" API in DifferentialRevisionQuery, and use it on the revision view
Summary: Ref T13639. Move away from "withPath(..., ...)" to "withPaths(...)".

Test Plan: {F8539323}

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21619
2021-03-15 16:16:11 -07:00
epriestley
26c68942bd Update "AffectedPath" table when a revision's repository changes
Summary:
Ref T13639. There's currently a hard-to-hit bug where editing the "Repository" of a revision doesn't update this index.

Instead: update the index on repository change, not just diff update.

Test Plan:
  - Updated a revision, used debug view to see index update.
  - Changed repository on a revision, used debug view to see index update.

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21618
2021-03-15 16:16:10 -07:00
epriestley
01ea84029d Update table schema for "AffectedPath" table
Summary:
Ref T13639. Make schema changes:

  - Make repositoryID nullable, for revisions with no repository.
  - Remove "epoch", which has no readers and no clear use.
  - Change the ordering of the key, since "pathID" has more unique values and no queries ever issue without it.

Test Plan:
  - Ran `bin/storage upgrade`, got a clean schema.
  - Reindexed all revisions with an external script.
  - Reviewed index via debug UI, saw appropriate index for non-repositoy revisions.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21617
2021-03-15 16:16:10 -07:00
epriestley
38ef910da8 Move "Affected Path" index updates to a separate class
Summary:
Ref T13639. Move operations related to updating the "AffectedPath" index to a dedicated class.

This change has no functional effect and only moves code.

Test Plan: Used an external script to rebuild every revision index; destroyed a revision with `bin/remove destroy`.

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21616
2021-03-15 16:16:09 -07:00
epriestley
e919b4c35a Add a debug view of the "Affected Path" index to Differential
Summary:
Ref T13639. The "Affected Path" table is currently hard to inspect: there's no UI, and using MySQL just gives you a bunch of IDs.

Add a simple UI and a debug-mode link to it.

Test Plan:
{F8539098}

{F8539099}

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21615
2021-03-15 16:16:09 -07:00
epriestley
c317d16bdd Lift peculiar side effect of path indexing out of indexer
Summary:
Ref T13639. Updating the affected path table has a peculiar side effect from D19426, which is a simplification of a peculiar side effect from earlier.

Don't condition Owners behavior on path index behavior.

Test Plan: Created a revision.

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21614
2021-03-15 16:16:08 -07:00
epriestley
bcd592cf7e Remove support for "paths" parameter in "differential.query"
Summary: See T13639. This change simplifies providing a more modern approach to querying this data via "differential.revision.search".

Test Plan: Called "differential.query" with paths (got an error) and without paths (got a valid query result).

Differential Revision: https://secure.phabricator.com/D21613
2021-03-15 16:16:08 -07:00
epriestley
e730f55e88 Retitle "Recently Open Revisions" panel to "Recent Open Revisions"
Summary:
Ref T13639. In D17754, this:

> OPEN REVISIONS
> Recently updated open revisions affecting this file.

...was simplified into:

> RECENTLY OPEN REVISIONS

This is a bit misleading, since the panel doesn't contain "recently open" results. Use "Recent Open" instead, which is a bit more consistent with other product text. This is still slightly misleading, but probably close enough.

Test Plan: Read text.

Maniphest Tasks: T13639

Differential Revision: https://secure.phabricator.com/D21612
2021-03-15 16:16:07 -07:00
epriestley
b11c6fcacd Clarify the behavior of "audit.can-author-close-audit"
Summary:
Ref T13631. This option has a behavior other than the behavior implied by the name and documented.

Document the correct behavior, at least. This can likely be removed after T10574.

Test Plan: Read config option help in Config.

Maniphest Tasks: T13631

Differential Revision: https://secure.phabricator.com/D21610
2021-03-12 09:20:16 -08:00
epriestley
4b529e6009 Fix a followup notification paging error with partial objects
Summary: Ref T13623. In D21603, I made the "partial object" this query returns a raw row, which paging keys can no longer be extracted from properly.

Test Plan: Paged notifications to page 2, no longer saw an error.

Maniphest Tasks: T13623

Differential Revision: https://secure.phabricator.com/D21609
2021-03-12 09:09:45 -08:00
epriestley
32da29b965 Provide more help around GRANT errors, particularly for missing TEMPORARY TABLE permission
Summary:
Fixes T13622. Figuring out what permissions we have seems difficult, so address this a bit more narrowly:

  - Make the "access denied" error message a bit more helpful.
  - Tailor error handling for the "CREATE TEMPORARY TABLE" statement.

Test Plan:
  - Created a new user, granted them "SELECT ON *.*" but not "CREATE TEMPORARY TABLE", ran `bin/storage upgrade --force --apply phabricator:20210215.changeset.02.phid-populate.php`.
  - Before: fairly opaque error.
  - After: fairly useful error.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13622

Differential Revision: https://secure.phabricator.com/D21608
2021-03-11 14:55:21 -08:00
epriestley
31c9d4094f Improve routing of "/robots.txt", "/favicon.ico", and "/status/" on Short and Blog sites
Summary: Ref T13636. Add routing for "/robots.txt", "favicon.ico", and "/status/" on the ShortSite and BlogSite.

Test Plan: Visted all resources (and 404 pages) on Short and Blog sites, and Platform site.

Maniphest Tasks: T13636

Differential Revision: https://secure.phabricator.com/D21607
2021-03-11 14:05:39 -08:00
epriestley
36c6eb9663 Improve routing of "/robots.txt", "/favicon.ico", "/status/", and 404 on custom Sites
Summary:
Fixes T12919. Fixes T13636. Prior to this change, some well-known resource paths don't route on sites like ResourceSite.

- `/robots.txt`: Make it route on ResourceSite and just deny the whole site.
- `/favicon.ico`: Make it route on ResourceSite.
- `/status/`: Make it route on ResourceSite.
- 404: Make it render a 404 on ResourceSite.

Test Plan:
  - Visited all URIs on ResourceSite, got sensible responses.
  - Visited all URIs on main site.
  - Visited 404 while logged out, got login page.

Maniphest Tasks: T13636, T12919

Differential Revision: https://secure.phabricator.com/D21606
2021-03-11 14:05:39 -08:00
epriestley
4484946cfd In JSON DocumentEngine, preserve the distinction between "{}" and "[]"
Summary:
Ref T13635. Currently, the JSON DocumentEngine uses "phutil_json_decode()", but this can confuse "{}" and "[]".

Be more careful about how the JSON value is decoded, to preserve the distinction.

Test Plan: {F8520479}

Maniphest Tasks: T13635

Differential Revision: https://secure.phabricator.com/D21605
2021-03-11 12:49:56 -08:00
epriestley
0815891e42 Fix an error when users receive notifications about objects they can no longer see
Summary:
Ref T13623. The change in D21577 could lead to a case where we try to access stories the user can't see.

Move the story-loading piece to "willFilterPage()" to make our way thorugh this.

Test Plan:
  - Made FeedStory return nothing to simulate invisible notifications, loaded page.
    - Before: index access fatal.
    - After: clean "no notifications".
  - Loaded notifications normally, saw normal notifications.

Maniphest Tasks: T13623

Differential Revision: https://secure.phabricator.com/D21603
2021-03-11 10:44:42 -08:00
epriestley
4cff4dc68b Interpret search tokens in the for "_..." as substring search
Summary: Ref T13632. Users searching for `__FILE__`, etc., almost certainly mean to perform a substring search.

Test Plan: Added tests and made them pass. Searched for various tokens, saw compiler interpretation in UI.

Maniphest Tasks: T13632

Differential Revision: https://secure.phabricator.com/D21602
2021-03-10 12:00:01 -08:00
epriestley
afdef332fb Allow "transaction.search" to be called on an object type
Summary:
Ref T13631. This supports a more robust version of "poll for updates by using dateModified window queries" that uses transactions as a logical clock.

This is particularly relevant for commits, since they don't have a "dateModified" at time of writing.

Test Plan:
  - Queried for transactions by type and object.
  - Issued various invalid transaction queries, got appropriate errors.

Maniphest Tasks: T13631

Differential Revision: https://secure.phabricator.com/D21601
2021-03-10 10:20:03 -08:00
epriestley
404b55ce57 Give audit statuses API constants that match their UI strings
Summary: Ref T13631. These strings were a little inconsistent; make them more consistent.

Test Plan: Called `diffusion.commit.search` with the appropriate attachment, saw slightly more consistent statuses.

Maniphest Tasks: T13631

Differential Revision: https://secure.phabricator.com/D21600
2021-03-10 10:20:03 -08:00
epriestley
ac2f5a1046 Modernize and clean up "PhabricatorAuditStatusConstants"
Summary:
Ref T13631. Move "PhabricatorAuditStatusConstants" to a more modern object ("PhabricatorAuditRequestStatus").

Expose the status value via Conduit.

Test Plan:
  - Ran `bin/audit delete`.
  - Viewed a commit with auditors in the web UI.
  - Grepped for affected symbols.
  - Called Conduit with the "auditors" attachment, saw auditor statuses.

Maniphest Tasks: T13631

Differential Revision: https://secure.phabricator.com/D21599
2021-03-10 09:21:55 -08:00
epriestley
2636d84d0c Remove very old Audit status constants and AuditRequest data
Summary:
Ref T13631. See that task for discussion.

  - "NONE": Probably never used?
  - "CC": Obsoleted by subscribers.
  - "AUDIT_NOT_REQUIRED": For Owners packages, obsoleted by edges.
  - "CLOSED": For "Close Audit", obsoleted by "Request Verification".

Test Plan:
  - Grepped for constants, browsed Diffusion.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13631

Differential Revision: https://secure.phabricator.com/D21598
2021-03-10 09:21:54 -08:00
epriestley
55532b3f74 Add a very basic "auditors" attachment to "differential.commit.search"
Summary: Ref T13631. For now, this only shows the auditor PHID. The current status constants could use some cleanup before they're exposed.

Test Plan: Queried with "auditors" attachment, saw basic auditor information.

Maniphest Tasks: T13631

Differential Revision: https://secure.phabricator.com/D21597
2021-03-10 09:21:54 -08:00
epriestley
bfe7cdc5a2 Provide default image alt text in more contexts and support custom alt text
Summary:
Ref T13629.

  - Allow files to have custom alt text.
  - If a file doesn't have alt text, try to generate a plausible default alt text with the information we have.

Test Plan:
  - Viewed image files in DocumentEngine diffs, files, `{Fxxx}` embeds, and lightboxes.
  - Saw default alt text in all cases, or custom alt text if provided.
  - Set, modified, and removed file alt text. Viewed timeline and feed.
  - Pulled alt text with "conduit.search".

Maniphest Tasks: T13629

Differential Revision: https://secure.phabricator.com/D21596
2021-03-04 16:51:23 -08:00
epriestley
fceb9a3f9e Hide the "Author" field in Differential on the web create workflow
Summary:
Ref T13628. When creating a revision, hide the recently introduced "Author" field.

It seems exceedingly unlikely that anyone would ever want to adjust this field when creating a revision, and this workflow already has a lot of fields and complexity, so this likely adds more noise than signal on the balance.

Test Plan:
  - Created revisions via copy/paste on the web workflow, no longer saw "Author".
  - Used "Edit Revision" to adjust the author normally.

Maniphest Tasks: T13628

Differential Revision: https://secure.phabricator.com/D21595
2021-03-04 15:50:15 -08:00
epriestley
a9704428ff In Audit, use repository identities to prevent author-auditors
Summary:
See PHI2015. Diffusion attempts to prevent a commit's author from being made an auditor, but currently uses an out-of-date method for identifying the author.

Use the modern ("Repository Identity" aware) method instead.

Test Plan:
  - Authored a commit as user "X", mapped to my account.
  - Pushed/imported/discovered it.
  - Changed the identity mapping for "X" from my account to a different account.
  - Tried to add myself as an auditor.
    - Before: error, "author can't be an auditor".
    - After: succeeds.
  - Tried to add the newly mapped user as an auditor. This correctly fails with the "author can't be an auditor" error.

It's possible to put commits into a wonky state by remapping the author identity to a user who is already an auditor, but I think that isn't important and we can't do much about it, realistically.

Differential Revision: https://secure.phabricator.com/D21594
2021-03-04 09:33:49 -08:00
epriestley
9b6a030292 Use the same icon for "Resigned" in Differential and Diffusion
Summary:
See PHI2015. Currently, "resigned" reviewers/auditors get different icons in Differential and Diffusion.

The Diffusion icon is exceptionally poor and confusing: it does not communicate "resigned" and it is similar to other icons.

For clarity and consistency, use the Differential icon (a grey "X") in both applications.

Test Plan: {F8492303}

Differential Revision: https://secure.phabricator.com/D21593
2021-03-04 09:33:48 -08:00
epriestley
f3caf7e9fc Improve rendering behavior for "foist upon" with an empty tokenizer
Summary:
Ref T13628. If you "Foist Upon", then delete the value in the tokenizer, then scroll down to the preview, you currently get an ugly "Unknown Object (???)" rendering.

This is technically correct in some vague sense and the transaction won't apply, but provide a more aesthetic rendering instead.

Test Plan: {F8487050}

Maniphest Tasks: T13628

Differential Revision: https://secure.phabricator.com/D21592
2021-03-03 16:08:26 -08:00
epriestley
dec1413684 Provide a more general "Author" transaction for Differential
Summary:
Ref T13628. Currently, Differential has a "Commandeer" action, but no way to edit the author otherwise.

This is largely archaic: there is no reason to prevent editing the author, and this makes it difficult to undo mistakes if you commandeer by accident.

Instead, provide a normal "Author" field and a "Foist Upon" action, similar to the "Owner" and "Claim/Assign" fields in Maniphest.

Test Plan:
  - Applied author transactions as the old author ("foisted"), the new author ("commandeerd"), and an arbitrary third party ("changed author").
  - Tried to unassign author, etc.
  - Viewed stories in feed and transaction timeline.
  - Saw sensible automatic reviewer changes.
  - Used existing "Commandeer" action, which is unchanged.
  - Called "transaction.search" and saw reasonable transaction values.

Maniphest Tasks: T13628

Differential Revision: https://secure.phabricator.com/D21591
2021-03-03 15:49:22 -08:00
Matthew Bowker
c0ac5be8a8 Provide new tool "bin/user approve" to approve an account.
Summary: T13578

Test Plan:
This method uses the existing transaction.  As such, most of the testing focused on the integration between the workflow and transaction.  The only change made to the transaction was to allow an omnipotent user to make the change in addition to an admin.

Other than that, I removed the "approved" flag from the user, then ran the command-line utilty until the user was successfully approved.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T13578

Differential Revision: https://secure.phabricator.com/D21587
2021-03-03 04:35:50 +00:00
epriestley
33bce22ef2 Never return external connections to the GlobalLock connection pool
Summary:
Ref T13627. If a lock fails, the connection may be returned to the pool, even if the connection is an external connection. Under old versions of MySQL, connection reuse can release other locks on the same connection.

Don't return external connections to the pool.

This issue was introduced in D21369.

Test Plan: Added a failing test and made it pass.

Maniphest Tasks: T13627

Differential Revision: https://secure.phabricator.com/D21585
2021-03-02 13:44:17 -08:00
epriestley
2b473558c2 Refuse to acquire a second GlobalLock on a connection
Summary:
Ref T13627. MySQL versions older than 5.7 release held locks when a new lock is acquired.

Prevent acquisition of a second lock to prevent this.

Test Plan: Added a failing unit test, made it pass.

Maniphest Tasks: T13627

Differential Revision: https://secure.phabricator.com/D21586
2021-03-02 13:44:17 -08:00
epriestley
466013f11a Prevent external connections from being mutated on held locks
Summary: Ref T13627. This makes the API harder to misuse: setting an external connection on a held lock isn't a meaningful operation. Prevent it.

Test Plan: Added a failing test, made it pass.

Maniphest Tasks: T13627

Differential Revision: https://secure.phabricator.com/D21584
2021-03-02 13:44:16 -08:00
epriestley
15dbf6bdf0 When a GlobalLock with an external connection is released, don't return it to the pool
Summary:
Ref T13627. Currently, global locks always return connections (even external connections) to the connection pool when unlocked.

This code is obviously buggy: `isExternalConnection` is set to false immediately before it is tested. This bug has existed since this code was introduced, in D15792.

  - Instead of storing a flag, store the actual connection.
  - Don't clear it when unlocking.
  - Don't return external connections to the pool.

Test Plan:
  - Added a failing test, made it pass.

Maniphest Tasks: T13627

Differential Revision: https://secure.phabricator.com/D21583
2021-03-02 13:44:16 -08:00