Error creating inline comment via conduit API

Question
#1

I’ m trying to create an inline comment via conduit but get an error:

>>> api.differential.createinline(revisionID=1, filePath=new_path, isNewFile=False, lineNumber=40, content=content)
Traceback (most recent call last):
  File "/Users/ericsalemi/Library/Application Support/JetBrains/Toolbox/apps/IDEA-C/ch-0/203.6682.168/IntelliJ IDEA CE.app.plugins/python-ce/helpers/pydev/_pydevd_bundle/pydevd_exec2.py", line 3, in Exec
    exec(exp, global_vars, local_vars)
  File "<input>", line 1, in <module>
  File "/Users/ericsalemi/vcs/ci/venv/lib/python3.9/site-packages/phabricator/__init__.py", line 253, in __call__
    return self._request(**kwargs)
  File "/Users/ericsalemi/vcs/ci/venv/lib/python3.9/site-packages/phabricator/__init__.py", line 315, in _request
    data = self._parse_response(response.text)
  File "/Users/ericsalemi/vcs/ci/venv/lib/python3.9/site-packages/phabricator/__init__.py", line 325, in _parse_response
    raise APIError(parsed['error_code'], parsed['error_info'])
phabricator.APIError: ERR-CONDUIT-CORE: #1366: Incorrect integer value: '' for column `bitnami_phabricator_differential`.`differential_transaction_comment`.`isNewFile` at row 1

The MySQL type for the isNewFile field is tinyint:

MariaDB [bitnami_phabricator_differential]> SELECT DATA_TYPE FROM INFORMATION_SCHEMA.COLUMNS  WHERE TABLE_NAME = 'differential_transaction_comment' AND COLUMN_NAME = 'isNewFile';
+-----------+
| DATA_TYPE |
+-----------+
| tinyint   |
+-----------+
1 row in set (0.005 sec)

I’m not sure where the problem is but because the isNewFile conduit parameter is required bool I assumed using the Python value False is the right thing to do. The fact the MySQL type is tinyint worries me and make me think there might be a type mismatch. For info, creating the inline comment via the UI works perfectly.

Phabricator version:

86ad69863930 (Oct 19 2020)

#2

If that makes things easier I reproduce the same error using arcanist directly:

> echo '{"revisionID": "1", "diffID": "6", "filePath": ".gitignore", "isNewFile": false, "lineNumber": 1, "content": "..."}' | arc call-conduit --conduit-uri *** --conduit-token *** -- differential.createinline
{
  "error": "ERR-CONDUIT-CORE",
  "errorMessage": "ERR-CONDUIT-CORE: <differential.createinline> #1366: Incorrect integer value: '' for column `bitnami_phabricator_differential`.`differential_transaction_comment`.`isNewFile` at row 1",
  "response": null
}
#3

Use “0” or “1” in for isNewFile

#4

@amckinley Your suggestion works when using arc but not when using the python phabricator API package since it does some runtime type checking and complains the value should be a bool. The problem seems to be that the isNewFile is marked as being required bool in the API but is actually backed by an tinyint, which makes it impossible to create an inline comment via the Python interface.

#5

My expectation is that client libraries should not do runtime type checking.

The upside seems very small: if their implementation is correct, you save perhaps 100ms per type error by receiving a client error instead of a server error.

The downside seems very large: if their implementation produces a false negative (a client type error where no server type error exists), you waste a day interacting with upstreams.

Some Conduit types also seem very challenging to parse, like list<map<string, wild>> (used by all modern *.edit) methods.

The upstream clients do not perform client-side typechecking and this doesn’t appear to lead to problems today.

Am I missing a reason (or set of reasons) that client-side type checking is a desirable feature that the upstream should endeavor to provide support for?

#6

@epriestley As far as I can see giving a JSON value false in the conduit call above (See comment 2) leads to an ERR-CONDUIT-CORE. Handing out a boolean value for a parameter which is marked as a required bool seems like a logical choice and it just fails. It feels counter intuitive to have to push an integer value instead, which could be ambiguous even though 0 for false seems straightforward enough.

Could upstream consider auto-casting a boolean to the expected integer value for the isNewFile field?

Otherwise I will most probably file a bug on the python package.

#7

This is a bug in the upstream in some sense, but there are a bunch of “XY Problem” issues to untangle here (clients likely should not do typechecking and the workaround is trivial if they do not; most use cases for differential.createinline are to submit lint messages, but there’s a preferred explicit pathway for lint; other theoretical use cases are not well-supported by existing API methods; and so on).

It’s not clear what your problem is, but the “best” solution is quite possibly not fixing this bug in the upstream.

If your problem is in the vein of “I want to submit lint messages”, triggering a lint build with Harbormaster and submitting messages via harbormaster.sendmessage provides better support.

If your problem is in the vein of “I want to synchronize inlines with (or submit inlines from) an external IDE”, the existing APIs are likely insufficient and fixing this bug won’t bring you much closer to a robust solution. For example, there is no API for interacting with edit suggestions today.

If your problem is very narrow and legitimately served by differential.createinline, the workaround is blocked only by client typechecking, and client typechecking has no real reason to exist, the best solution is to have the client you prefer to use remove typechecking and then use the trivial workaround, since this resolution solves a larger class of issues (all cases where older Conduit API methods differ in their human-readable type and accepted type and a very minor problem is turned into a much larger problem by typechecking).

#8

@epriestley Thanks for sharing your thoughts and concerns.

Currently I’m building a Python script whose purpose is to migrate the content of an existing Gitlab server to Phabricator focusing on issues and merge request. So in short I’m using conduit wherever applicable to speed up the process. I’m specifically using differential.createinline to reproduce inline comments found in Gitlab merge requests. Using arc calls directly works and bypasses the Python client’s type check so I’m not blocked anymore.

#9

I fixed this narrowly in https://secure.phabricator.com/D21522, now in master.

Note that differetial.createinline creates draft inlines which are only visible to the author. It will also suffer from the general issues associated with doing imports via API (like authors and creation dates being incorrect). See https://secure.phabricator.com/T3179 for broader discussion of imports.

See also https://secure.phabricator.com/T12678 for more general discussion of the type interface between MySQL and PHP.

#10

@epriestley Thanks for the swift actions.

Indeed I noticed that differential.createinline create the comments but they still need to be submitted at the level of the revision. Is there a conduit method for the submit action? Otherwise I’m currently using extra selenium logic to simulate clicks when conduit isn’t sufficient.