4988: Fail to post diff with perforce repo: 'Value to convert is unexpected type %s', <class 'rev iewboard.scmtools.core.Revision'>

benjackson

What version are you running?

4.0.11

What's the URL of the page containing the problem?

│>>> Making HTTP POST request to http://ukfd-ltp4l:49320/api/validation/diffs/

What steps will reproduce the problem?

It doesn't seem to happen for all diffs where a new file is added, but it's certainly related to adding new files, with perforce as the repo type. Specifically, it needs to go though these lines in perforce.py where the old revision is set to PRE_CREATION.

        # Older versions of Perforce had this lovely idiosyncracy that diffs
        # show revision #1 both for pre-creation and when there's an actual
        # revision. In this case, we need to check if the file already exists
        # in the repository.
        #
        # Newer versions use #0, so it's quicker to check.
        if (revision == b'0' or
            (revision == b'1' and
             not self.repository.get_file_exists(filename.decode('utf-8'),
                                                 revision.decode('utf-8')))):
            revision = PRE_CREATION

  1. Using git-p4 client repo, add a new file and commit that
  2. rbt post

As i said, i debugged it a bit but wasn't 100% sure why this didn't happen for every new file add. See below for the exact exception and code which is triggering it. Attached screenshots of debugger showing the call stacks where it's assigned etc.

I can easily repro this with a specific commit right now, and have a debug environment set up, so please shout if there is more info I can provide.

What is the expected output? What do you see instead?

Diff created.
Actual: Exceptoin TypeError: ('Value to convert is unexpected type %s', <class 'reviewboard.scmtools.core.Revision'>)

$>rbt post --repository perforce:1666 --server http://ukfd-ltp4l:49320 675417754ac980cc60d9b72eddaf9f9cb80817dd

Unexpected error when validating the diff: ('Value to convert is unexpected type %s', <class 'reviewboard.scmtools.core.Revision'>) (API Error 224: Diff Parsing Failed)
ukfd-ltp4l(benj:TESTKIT_main.git) TESTKIT/main.git>

What operating system are you using? What browser?

RHEL7

Also reproduced in a minimal ubuntu 20.02 container (based on contrib/docker/Dockerfile)

Please provide any additional information below.

The issue appears to be with this code in filediff_creator.py:

# Store the information on the parent's filename and revision.
            # It's important we force these to text, since they may be
            # byte strings and the revision may be a Revision instance.
            #
            # Starting in Review Board 4.0.5, we store this any time there's
            # a parent diff, whether or not the file existed in the parent
            # diff.
            extra_data.update({
                FileDiff._IS_PARENT_EMPTY_KEY: parent_is_empty,
                'parent_source_filename':
                    convert_to_unicode(parent_source_filename,
                                       encoding_list)[1],
                'parent_source_revision':
                    convert_to_unicode(parent_source_revision,
                                       encoding_list)[1],
            })

Per the comment, the argument parent_source_revision may be a Revision object (in this case, it is literally PRE_CREATION which is an instance of Revision, however convert_to_unicode requires arguments to be some type of string, and throws an exception when they aren'

Git blame points the finger at this commit on the branch: b972f20c55 (it's the same on master)


Here's the full stack trace from the log:

2023-01-20 20:59:32,986 - ERROR -  - reviewboard.webapi.resources.validate_diff - Unexpected error whe
n validating diff.
Traceback (most recent call last):
  File "/build_root/reviewboard/reviewboard/webapi/resources/validate_diff.py", line 161, in create
    DiffSet.objects.create_from_upload(
  File "/build_root/reviewboard/reviewboard/diffviewer/managers.py", line 767, in create_from_upload
    return self.create_from_data(
  File "/build_root/reviewboard/reviewboard/diffviewer/managers.py", line 1060, in create_from_data
    create_filediffs(
  File "/build_root/reviewboard/reviewboard/diffviewer/filediff_creator.py", line 210, in create_filed
iffs
    convert_to_unicode(parent_source_revision,
  File "/build_root/reviewboard/reviewboard/diffviewer/diffutils.py", line 112, in convert_to_unicode
    raise TypeError('Value to convert is unexpected type %s', type(s))
TypeError: ('Value to convert is unexpected type %s', <class 'reviewboard.scmtools.core.Revision'>)

Full output from rbt with --debug:

>>> RBTools 3.1.3 alpha 0 (dev)
>>> Python 3.6.3 (default, Apr 10 2019, 14:37:36)
[GCC 4.8.5 20150623 (Red Hat 4.8.5-16)]
>>> Running on Linux-3.10.0-1160.59.1.el7.x86_64-x86_64-with-redhat-7.9-Maipo
>>> Home = /mma/users/benj
>>> Current directory = /mma/users/benj/proj/perforce-1666/TESTKIT/main.git
>>> Making HTTP GET request to http://ukfd-ltp4l:49320/api/
>>> Unable to execute "cleartool help": skipping ClearCase
>>> Running: tf vc help
>>> Checking for a Bazaar repository...
>>> Unable to execute "brz help" or "bzr help": skipping Bazaar
>>> Checking for a VersionVault / ClearCase repository...
>>> Unable to execute "cleartool help": skipping ClearCase
>>> Checking for a CVS repository...
>>> Checking for a Git repository...
>>> Running: git rev-parse --git-dir
>>> Running: git config core.bare
>>> Running: git rev-parse --show-toplevel
>>> Checking for a Mercurial repository...
>>> Unable to execute "hg --help": skipping Mercurial
>>> Checking for a Perforce repository...
>>> Running: p4 info
>>> Checking for a Plastic repository...
>>> Unable to execute "cm version": skipping Plastic
>>> Checking for a Cliosoft SOS repository...
>>> Running: soscmd version
>>> Unable to execute "soscmd version"; skipping SOS
>>> Checking for a Subversion repository...
>>> Running: svn --non-interactive info
>>> Command exited with rc 1: ['svn', '--non-interactive', 'info']
["svn: E155007: '/mma/users/benj/proj/perforce-1666/TESTKIT/main.git' is not a working copy\n"]---
>>> Checking for a Team Foundation Server repository...
>>> Unable to execute "tf help": skipping TFS
>>> Finding deepest repository of multiple matching repository types.
>>> Running: git rev-parse --git-dir
>>> Running: git config core.bare
>>> Running: git rev-parse --show-toplevel
>>> Running: git symbolic-ref -q HEAD
>>> Running: git show-ref --verify refs/remotes/p4/master
>>> Running: git config --get git-p4.port
>>> Repository info: Path: perforce:1666, Base path:
>>> Making HTTP GET request to http://ukfd-ltp4l:49320/api/repositories/?name=perforce%3A1666&only-fields=id%2Cname%2Cmirror_path%2Cpath&only-links=info&tool=Git%2CPerforce%2CSubversion
>>> HTTP GET request to http://ukfd-ltp4l:49320/api/repositories/?name=perforce%3A1666&only-fields=id%2Cname%2Cmirror_path%2Cpath&only-links=info&tool=Git%2CPerforce%2CSubversion cannot be cached
>>> Making HTTP GET request to http://ukfd-ltp4l:49320/api/repositories/1/info/
>>> Got API Error 209 (HTTP code 501): The specified repository is not able to perform this action.
>>> Error data: {'err': {'code': 209, 'msg': 'The specified repository is not able to perform this action.'}, 'stat': 'fail'}
>>> Command line: rbt post --debug --repository perforce:1666 --server http://ukfd-ltp4l:49320 675417754ac980cc60d9b72eddaf9f9cb80817dd
>>> Running: git rev-parse 675417754ac980cc60d9b72eddaf9f9cb80817dd
>>> Running: git rev-parse 675417754ac980cc60d9b72eddaf9f9cb80817dd^
>>> Running: git branch --remotes
>>> Running: git rev-list f84fc277594e6098d370ddb49431abde0f11f4c7 --not --remotes=p4
>>> Running: git rev-parse 8d7c75c93cbf0b8c11bb47ca602676b8b5089165^
>>> Found youngest remote git commit b91de936426df9dbc694f37d033d213537b95de6
>>> Running: git version
>>> Running: git -c core.quotepath=false diff --no-color --no-prefix -r -u --no-ext-diff f84fc277594e6098d370ddb49431abde0f11f4c7..675417754ac980cc60d9b72eddaf9f9cb80817dd
>>> Running: git log b91de936426df9dbc694f37d033d213537b95de6
>>> Running: p4 files //depot/TESTKIT/main/src/tcl/testKit_Packages.tcl@2485266
>>> Running: p4 files //depot/TESTKIT/main/src/tcl/testKit_TestLibCaseSettings.tcl@2485266
>>> Running: git -c core.quotepath=false diff --no-color --no-prefix -r -u --no-ext-diff b91de936426df9dbc694f37d033d213537b95de6..f84fc277594e6098d370ddb49431abde0f11f4c7
>>> Running: git log b91de936426df9dbc694f37d033d213537b95de6
>>> Running: p4 files //depot/TESTKIT/main/src/scripts/testKit_Test@2485266
>>> Running: p4 files //depot/TESTKIT/main/src/tcl/testKit_TestLibFoundation.tcl@2485266
>>> Running: p4 files //depot/TESTKIT/main/src/tcl/testKit_TestLibReconfigure.tcl@2485266
>>> Running: p4 files //depot/TESTKIT/main/src/tcl/testKit_TestLibRunning.tcl@2485266
>>> Running: p4 files //depot/TESTKIT/main/src/tcl/testKit_TestLibSymbolExpansion.tcl@2485266
>>> Generated parent diff size: 8364 bytes
>>> Making HTTP GET request to http://ukfd-ltp4l:49320/api/validation/diffs/
>>> HTTP GET request to http://ukfd-ltp4l:49320/api/validation/diffs/ cannot be cached
>>> Making HTTP POST request to http://ukfd-ltp4l:49320/api/validation/diffs/

Please log in to the Review Board server at ukfd-ltp4l:49320.
Username: Ben.Jackson
Password:
>>> Got API Error 224 (HTTP code 400): Unexpected error when validating the diff: ('Value to convert is unexpected type %s', <class 'reviewboard.scmtools.core.Revision'>)
>>> Error data: {'err': {'code': 224, 'msg': "Unexpected error when validating the diff: ('Value to convert is unexpected type %s', <class 'reviewboard.scmtools.core.Revision'>)"}, 'stat': 'fail'}
Traceback (most recent call last):
  File "/mma/users/benj/proj/rbtools/rbtools/api/request.py", line 827, in make_request
    request.url, body, headers, request.method))
  File "/mma/users/benj/proj/rbtools/rbtools/api/cache.py", line 209, in make_request
    return self.urlopen(request)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 223, in urlopen
    return opener.open(url, data, timeout)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 532, in open
    response = meth(req, response)
  File "/mma/users/benj/proj/rbtools/rbtools/api/request.py", line 370, in http_response
    response.info())
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 564, in error
    result = self._call_chain(*args)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 504, in _call_chain
    result = func(*args)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 1028, in http_error_401
    url, req, headers)
  File "/mma/users/benj/proj/rbtools/rbtools/api/request.py", line 458, in http_error_auth_reqed
    self, authreq, host, req, headers)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 981, in http_error_auth_reqed
    return self.retry_http_basic_auth(host, req, realm)
  File "/mma/users/benj/proj/rbtools/rbtools/api/request.py", line 546, in retry_http_basic_auth
    return self.parent.open(request, timeout=request.timeout)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 532, in open
    response = meth(req, response)
  File "/mma/users/benj/proj/rbtools/rbtools/api/request.py", line 370, in http_response
    response.info())
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 570, in error
    return self._call_chain(*args)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 504, in _call_chain
    result = func(*args)
  File "/opt/rh/rh-python36/root/usr/lib64/python3.6/urllib/request.py", line 650, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 400: Bad Request

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/mma/users/benj/proj/rbtools/rbtools/commands/post.py", line 959, in main
    self._validate_squashed_diff(squashed_diff)
  File "/mma/users/benj/proj/rbtools/rbtools/commands/post.py", line 1569, in _validate_squashed_diff
    **validate_kwargs)
  File "/mma/users/benj/proj/rbtools/rbtools/api/decorators.py", line 27, in request_method
    *args, **kwargs)
  File "/mma/users/benj/proj/rbtools/rbtools/api/transport/sync.py", line 83, in execute_request_method
    return self._execute_request(request)
  File "/mma/users/benj/proj/rbtools/rbtools/api/transport/sync.py", line 92, in _execute_request
    rsp = self.server.make_request(request)
  File "/mma/users/benj/proj/rbtools/rbtools/api/request.py", line 829, in make_request
    self.process_error(e.code, e.read())
  File "/mma/users/benj/proj/rbtools/rbtools/api/request.py", line 803, in process_error
    rsp['err']['msg'])
rbtools.api.errors.BadRequestError: Unexpected error when validating the diff: ('Value to convert is unexpected type %s', <class 'reviewboard.scmtools.core.Revision'>) (API Error 224: Diff Parsing Failed)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/mma/users/benj/.local/bin/rbt", line 33, in <module>
    sys.exit(load_entry_point('RBTools', 'console_scripts', 'rbt')())
  File "/mma/users/benj/proj/rbtools/rbtools/commands/main.py", line 207, in main
    command.run_from_argv([RB_MAIN, command_name] + args)
  File "/mma/users/benj/proj/rbtools/rbtools/commands/__init__.py", line 1102, in run_from_argv
    exit_code = self.main(*args) or 0
  File "/mma/users/benj/proj/rbtools/rbtools/commands/post.py", line 970, in main
    % (msg_prefix, e))
rbtools.commands.CommandError: Error validating diff
#1 benjackson

I just tried with release-5.0.x and same behaviour.

#2 benjackson

https://reviews.reviewboard.org/r/12798/
https://reviews.reviewboard.org/r/12799/

david
#3 david

Fixed in release-4.0.x (99b1e7e).

This will ship in the upcoming 5.0.2 and (at a later point), 4.0.12

chipx86
#4 chipx86

Found a regression with the fix, but we'll get reworked version in for 5.0.2 (which is going out tonight/tomorrow).

force_text()/force_str() will assume an encoding if the object is a bytes or converts to a bytes. The problem with this is that we can't assume at this particular stage. We instead need to use the configured encoding list. This matters particularly for people working with some Chinese encodings. convert_to_unicode() is responsible for trying the configured encodings, but since the fix was passing in the forced-encode, it never got a chance.

That's not a problem for Revision instances, but is for raw revisions. Probably not even a problem there (revisions are generally effectively ASCII in most SCMs), but it's more of a problem when forcing the encoding for filenames (which are completely at the whim of the developers committing code).

Making a change to specifically check for and cast the Revision before storing it instead, which should be a bit safer.

david
#5 david
  • -New
    +Fixed