views: fix local variable 'pid' referenced before assignment

[pazembrz]

pid variable does not exist as SQLAlchemyError can happen only during pid variable assignment so it shouldn't be used when handling this exception.

• INSPIR-3282

[pazembrz]

This fixes error we found when db connection fails

OperationalError: FATAL:  remaining connection slots are reserved for non-replication superuser connections

  File "sqlalchemy/engine/base.py", line 2276, in _wrap_pool_connect
    return fn()
  File "sqlalchemy/pool/base.py", line 363, in connect
    return _ConnectionFairy._checkout(self)
  File "sqlalchemy/pool/base.py", line 773, in _checkout
    fairy = _ConnectionRecord.checkout(pool)
  File "sqlalchemy/pool/base.py", line 497, in checkout
    rec._checkin_failed(err)
  File "sqlalchemy/util/langhelpers.py", line 68, in __exit__
    compat.reraise(exc_type, exc_value, exc_tb)
  File "sqlalchemy/util/compat.py", line 153, in reraise
    raise value
  File "sqlalchemy/pool/base.py", line 494, in checkout
    dbapi_connection = rec.get_connection()
  File "sqlalchemy/pool/base.py", line 606, in get_connection
    self.__connect()
  File "sqlalchemy/pool/base.py", line 652, in __connect
    connection = pool._invoke_creator(self)
  File "sqlalchemy/engine/strategies.py", line 114, in connect
    return dialect.connect(*cargs, **cparams)
  File "sqlalchemy/engine/default.py", line 489, in connect
    return self.dbapi.connect(*cargs, **cparams)
  File "__init__.py", line 126, in connect
    conn = _connect(dsn, connection_factory=connection_factory, **kwasync)

OperationalError: (psycopg2.OperationalError) FATAL:  remaining connection slots are reserved for non-replication superuser connections

(Background on this error at: http://sqlalche.me/e/e3q8)
  File "invenio_records_rest/views.py", line 383, in inner
    pid, record = request.view_args['pid_value'].data
  File "werkzeug/utils.py", line 90, in __get__
    value = self.func(obj)
  File "invenio_records_rest/utils.py", line 154, in data
    return self.resolver.resolve(self.value)
  File "inspirehep/pidstore/resolvers.py", line 26, in resolve
    pid = PersistentIdentifier.get(self.pid_type, pid_value)
  File "invenio_pidstore/models.py", line 194, in get
    return cls.query.filter_by(**args).one()
  File "sqlalchemy/orm/query.py", line 3347, in one
    ret = self.one_or_none()
  File "sqlalchemy/orm/query.py", line 3316, in one_or_none
    ret = list(self)
  File "sqlalchemy/orm/query.py", line 3389, in __iter__
    return self._execute_and_instances(context)
  File "sqlalchemy/orm/query.py", line 3411, in _execute_and_instances
    querycontext, self._connection_from_session, close_with_result=True
  File "sqlalchemy/orm/query.py", line 3426, in _get_bind_args
    mapper=self._bind_mapper(), clause=querycontext.statement, **kw
  File "sqlalchemy/orm/query.py", line 3404, in _connection_from_session
    conn = self.session.connection(**kw)
  File "sqlalchemy/orm/session.py", line 1133, in connection
    execution_options=execution_options,
  File "sqlalchemy/orm/session.py", line 1139, in _connection_for_bind
    engine, execution_options
  File "sqlalchemy/orm/session.py", line 432, in _connection_for_bind
    conn = bind._contextual_connect()
  File "sqlalchemy/engine/base.py", line 2242, in _contextual_connect
    self._wrap_pool_connect(self.pool.connect, None),
  File "sqlalchemy/engine/base.py", line 2280, in _wrap_pool_connect
    e, dialect, self
  File "sqlalchemy/engine/base.py", line 1547, in _handle_dbapi_exception_noconnection
    util.raise_from_cause(sqlalchemy_exception, exc_info)
  File "sqlalchemy/util/compat.py", line 398, in raise_from_cause
    reraise(type(exception), exception, tb=exc_tb, cause=cause)
  File "sqlalchemy/util/compat.py", line 152, in reraise
    raise value.with_traceback(tb)
  File "sqlalchemy/engine/base.py", line 2276, in _wrap_pool_connect
    return fn()
  File "sqlalchemy/pool/base.py", line 363, in connect
    return _ConnectionFairy._checkout(self)
  File "sqlalchemy/pool/base.py", line 773, in _checkout
    fairy = _ConnectionRecord.checkout(pool)
  File "sqlalchemy/pool/base.py", line 497, in checkout
    rec._checkin_failed(err)
  File "sqlalchemy/util/langhelpers.py", line 68, in __exit__
    compat.reraise(exc_type, exc_value, exc_tb)
  File "sqlalchemy/util/compat.py", line 153, in reraise
    raise value
  File "sqlalchemy/pool/base.py", line 494, in checkout
    dbapi_connection = rec.get_connection()
  File "sqlalchemy/pool/base.py", line 606, in get_connection
    self.__connect()
  File "sqlalchemy/pool/base.py", line 652, in __connect
    connection = pool._invoke_creator(self)
  File "sqlalchemy/engine/strategies.py", line 114, in connect
    return dialect.connect(*cargs, **cparams)
  File "sqlalchemy/engine/default.py", line 489, in connect
    return self.dbapi.connect(*cargs, **cparams)
  File "__init__.py", line 126, in connect
    conn = _connect(dsn, connection_factory=connection_factory, **kwasync)

UnboundLocalError: local variable 'pid' referenced before assignment
  File "flask/app.py", line 2446, in wsgi_app
    response = self.full_dispatch_request()
  File "flask/app.py", line 1951, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "flask_cors/extension.py", line 161, in wrapped_function
    return cors_after_request(app.make_response(f(*args, **kwargs)))
  File "flask/app.py", line 1820, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "flask/_compat.py", line 39, in reraise
    raise value
  File "flask/app.py", line 1949, in full_dispatch_request
    rv = self.dispatch_request()
  File "flask/app.py", line 1935, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "flask/views.py", line 89, in view
    return self.dispatch_request(*args, **kwargs)
  File "invenio_rest/views.py", line 240, in dispatch_request
    *args, **kwargs
  File "flask/views.py", line 163, in dispatch_request
    return meth(*args, **kwargs)
  File "invenio_records_rest/views.py", line 386, in inner
    raise PIDResolveRESTError(pid)

[drjova]

cc @inveniosoftware/architects

[slint]

There's apparently https://github.com/inveniosoftware/invenio-records-rest/pull/253, which seems to be fixing this issue (the problem is that the pid variable is not assigned in case of an exception, so it's replaced with pid_value which definitely exists and still allows having a meaningful error payload).

@egabancho did this as a backport to the maint-1.4 branch, so maybe he has more context on why it's not in master (it should go in though).

I think the true issue, also related to the stacktrace you posted, is that regular DB errors (like e.g. in this case reaching the maximum DB connections) are captured by the except SQLAlchemyError: clause and are re-raised and hidden as a PIDResolveRESTError error, which of course is horrible behaviour...

Besides integrating the fix that was mentioned in the above PR, I think the way to proceed is to figure out the errors that we care about and that are actually meant to be caught in this try/except, and only catch these specifically (probably there's a PersistentIdetnifiery.query.filter(...).one() at some point, which fails and gives back a NoResultFound exception). Good candidates for these exceptions in this case would be in invenio_pidstore.errors.

Also IMO for errors that can't actually be handled by the application (e.g. max DB connections reached), letting an exception bubble up and result to a "natural" 500 response is the correct behaviour (and also preserves the original exception with its required stacktrace).

[pazembrz]

I allowed myself to dive with PDB to this code and I am affraid that pid_value you want to pass in there is exatly the same pid_value from request.view_args:

> /home/pazembrz/.cache/pypoetry/virtualenvs/inspirehep-rAUWmTjo-py3.6/lib/python3.6/site-packages/invenio_records_rest/views.py(385)inner()
-> pid, record = request.view_args['pid_value'].data
(Pdb) print(pid_value)
<invenio_records_rest.utils.LazyPIDValue object at 0x7f10d27939b0>
(Pdb) print(request.view_args['pid_value'])
<invenio_records_rest.utils.LazyPIDValue object at 0x7f10d27939b0>

So it won't display any useful error when you will put pid_value there, instead it will just create a string representation of LazyPidValue object. I believe that solution here would be actually to remove try/except and let DB errors raise.