Page MenuHomePhabricator

Move symbols to be repository-based
ClosedPublic

Authored by joshuaspence on Apr 29 2015, 1:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 15, 7:45 AM
Unknown Object (File)
Sat, Dec 14, 12:25 PM
Unknown Object (File)
Wed, Dec 11, 9:51 PM
Unknown Object (File)
Wed, Dec 11, 3:58 AM
Unknown Object (File)
Sun, Dec 8, 10:26 AM
Unknown Object (File)
Fri, Dec 6, 8:33 PM
Unknown Object (File)
Fri, Dec 6, 4:17 AM
Unknown Object (File)
Thu, Dec 5, 12:38 PM

Details

Summary

Fixes T7220. Ref T7977. Changes symbols from being bound to an Arcanist project to being bound to a repository.

Test Plan
  • Added symbols and then applied migrations, symbols seemed to be migrated successfully.
  • Tested the /diffusion/symbol/$SYMBOL_NAME endpoint.
  • Tested the /diffusion/symbol/$SYMBOL_NAME endpoint with the ?repositories=$REPOSITORY_PHID parameter.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 5586
Build 5605: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence retitled this revision from to Move symbols to be repository-based.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)

Couple of notes.

resources/sql/autopatches/20150429.repositorysymbols.sql
3

Although I don't think this feature is too heavily used, it would be nice to try to migrate, and doesn't seem too difficult:

  • add new column
  • for each project which exists, issue an UPDATE to populate its repositoryPHID
  • drop old column
scripts/symbols/import_project_symbols.php
10

This file should be renamed.

30–37

We could probably just hard code this to like 4K or 8K or something reasonable-ish.

src/applications/diffusion/controller/DiffusionSymbolController.php
33

This should use RepositoryQuery and be policy-aware.

104

Consider getMonogram() in new code to prepare for making callsigns optional eventually.

src/applications/diffusion/query/DiffusionSymbolQuery.php
74

Maybe modernize to with... while the method name is changing anyway.

246

Should be RepositoryQuery

joshuaspence marked 5 inline comments as done.
joshuaspence edited edge metadata.

Changes as requested

I'm currently getting Bad getter call: getRepositoryPHID when trying to hit DiffusionSymbolController.

src/applications/repository/storage/PhabricatorRepositorySymbol.php
10–11

Maybe because this class removes arcanistProjectID but doesn't add protected $repositoryPHID?

src/applications/repository/storage/PhabricatorRepositorySymbol.php
10–11

Ah thanks.

epriestley edited edge metadata.

This also fixes T7977. Caught some minor junk inline.

resources/sql/autopatches/20150429.repositorysymbols.2.php
15 ↗(On Diff #30356)

Ignoring it and continuing is fine/reasonable/correct as far as I'm concerned.

scripts/symbols/clear_repository_symbols.php
28 ↗(On Diff #30356)

$more undefined, duplicates intent above?

29 ↗(On Diff #30356)

Use RepositoryQuery.

33–40 ↗(On Diff #30356)

Duplicate code.

This revision is now accepted and ready to land.May 1 2015, 6:18 PM

Ha, I have this change about 80% ready for review...

Sorry, I should have caught that quicker!

that's cool, I'll just compare the two :)

Looks like we started on opposite ends, which is good.
My version adds (diff):

  • Move "Use Symbols From" and "Languages" to repository object; Updated the JS and DiffusionBrowseFileController accordingly

I can rebase off this, and submit as a separate revision.

Looks like we started on opposite ends, which is good.
My version adds (diff):

  • Move "Use Symbols From" and "Languages" to repository object; Updated the JS and DiffusionBrowseFileController accordingly

I can rebase off this, and submit as a separate revision.

Ah that's true, I didn't even think about those.

joshuaspence edited edge metadata.
joshuaspence added inline comments.
resources/sql/autopatches/20150429.repositorysymbols.2.php
15 ↗(On Diff #30356)

I'm not even sure how this works exactly, because the repositoryPHID field is marked NOT NULL.

resources/sql/autopatches/20150429.repositorysymbols.2.php
15 ↗(On Diff #30356)

It will default to the empty string if it's marked NOT NULL (and has a VARCHAR, VARBINARY, TEXT, etc., type).

I just tested ./bin/remove destroy and it seems that I broke it.

Destroying PhabricatorRepository rARC...
[2015-05-03 11:36:05] EXCEPTION: (AphrontSchemaQueryException) #1054: Unknown column 'id' in 'where clause' at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:305]
arcanist(head=master, ref.master=977baacc329c), phabricator(head=master, ref.master=a8c29736fb32), phutil(head=master, ref.master=00e1f2da2829)
  #0 AphrontBaseMySQLDatabaseConnection::throwQueryCodeException(integer, string) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:275]
  #1 AphrontBaseMySQLDatabaseConnection::throwQueryException(mysqli) called at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:181]
  #2 AphrontBaseMySQLDatabaseConnection::executeRawQuery(string) called at [<phutil>/src/xsprintf/queryfx.php:6]
  #3 queryfx(AphrontMySQLiDatabaseConnection, string, string, string, NULL)
  #4 call_user_func_array(string, array) called at [<phutil>/src/aphront/storage/connection/AphrontDatabaseConnection.php:26]
  #5 AphrontDatabaseConnection::query(string, string, string, NULL) called at [<phabricator>/src/infrastructure/storage/lisk/LiskDAO.php:1176]
  #6 LiskDAO::delete() called at [<phabricator>/src/applications/repository/storage/PhabricatorRepository.php:1172]
  #7 PhabricatorRepository::delete() called at [<phabricator>/src/applications/repository/storage/PhabricatorRepository.php:1876]
  #8 PhabricatorRepository::destroyObjectPermanently(PhabricatorDestructionEngine) called at [<phabricator>/src/applications/system/engine/PhabricatorDestructionEngine.php:40]
  #9 PhabricatorDestructionEngine::destroyObject(PhabricatorRepository) called at [<phabricator>/src/applications/system/management/PhabricatorSystemRemoveDestroyWorkflow.php:98]
  #10 PhabricatorSystemRemoveDestroyWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:396]
  #11 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:292]
  #12 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/setup/manage_remove.php:21]
PHP Fatal error:  Uncaught exception 'Exception' with message 'Process exited with an open transaction! The transaction will be implicitly rolled back. Calls to openTransaction() must always be paired with a call to saveTransaction() or killTransaction().' in /usr/src/libphutil/src/aphront/storage/connection/AphrontDatabaseTransactionState.php:69
Stack trace:
#0 [internal function]: AphrontDatabaseTransactionState->__destruct()
#1 {main}
  thrown in /usr/src/libphutil/src/aphront/storage/connection/AphrontDatabaseTransactionState.php on line 69

Fatal error: Uncaught exception 'Exception' with message 'Process exited with an open transaction! The transaction will be implicitly rolled back. Calls to openTransaction() must always be paired with a call to saveTransaction() or killTransaction().' in /usr/src/libphutil/src/aphront/storage/connection/AphrontDatabaseTransactionState.php:69
Stack trace:
#0 [internal function]: AphrontDatabaseTransactionState->__destruct()
#1 {main}
  thrown in /usr/src/libphutil/src/aphront/storage/connection/AphrontDatabaseTransactionState.php on line 69
joshuaspence marked 3 inline comments as done.

Fix ./bin/remove destroy

Update datestamps on migrations

This revision was automatically updated to reflect the committed changes.