API AssertionError could be adapted to a http code

Bug #413174 reported by Curtis Hovey
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
Low
Unassigned

Bug Description

OOPS-1318S626 shows that a user tried to create a release for a milestone that had a release. The AssertionError is correct--the author of the script should have checked for the existence of the release before trying to create it. However, this oops does not represent an launchpad application error. We do not think the oops should have been reported in this context. Maybe a 401 or 403 should have been returned.

I think we want to register an adapter for AssertionError on the api host that sets a http code and shows the error message text instead of reporting a oops. The oops exists to tell us that the web app has a programming error, we want users to fix their own scripts.

OOPS-1694EB1486 is another oops that would benefit from such adapter.

Related branches

Revision history for this message
Curtis Hovey (sinzui) wrote :

If adpation is the right way to solve API assertion error oopes in the registry, I think it will automatically fix this problem in all launchpad.

summary: - API AssertionError creating a release
+ API AssertionError could be adapted to a http code
description: updated
Revision history for this message
Gary Poster (gary) wrote :

The adaptation makes sense to me.

Changed in launchpad-foundations:
status: New → Triaged
importance: Undecided → High
milestone: none → 10.03
Revision history for this message
William Grant (wgrant) wrote :

The assertion text will be returned to users -- isn't it possible that a blanket conversion to non-OOPSes will reveal something that shouldn't be revealed?

Revision history for this message
Curtis Hovey (sinzui) wrote :

The assertion message in this case is an instruction to the engineer that he has not read the documentation. It should be a statement like "You cannot create a release for a milestone that already has one." It is possible for a engineer to have decided to interpolate a private information and a reviewer approved it, but this can also be said about emails and notifications. AssertionErrors are not assumed to be about data, but contract violation.

Revision history for this message
Gary Poster (gary) wrote :

We could popularize the view that asserts should generate user visible text, and do an audit as part of this effort.

Alternatively, we could create our own assertion callable that had a name that made it clear that it would generate publicly visible errors ("def assertPublicly(bool, message=None)" for instance), and convert to that. It would raise a different error than AssertionError ("PublicAssertionError" for instance). We would register the adapter for this exception. We might register an adapter for the AssertionError that told users what they could do to ask us for a more informative error ("make a bug requesting that this use 'assertPublicly'").

The first is easier to do now, easier to use in the future, and easier to forget. The advantage of the second is explicitness.

We typically err on the side of explicitness and security. Perhaps we should do so now.

I'm feeling this is far-reaching enough into developer-land that it should be decided by the reviewers, or Björn, or Francis.

If it's my call...I suppose I'd do the more explicit/more secure approach. I don't feel strongly about it though.

Revision history for this message
Gary Poster (gary) wrote :

Francis prefers the simple solution now over the more complex solution never. That's a +1 in my book.

Gary Poster (gary)
Changed in launchpad-foundations:
milestone: 10.03 → 10.04
Changed in launchpad-foundations:
milestone: 10.04 → none
description: updated
Revision history for this message
Benji York (benji) wrote :

After discussing this Gary and I have some pre-implementation notes
about this bug:

The assert statements in the referenced OOPS reports should be changed
to raise a different exception (ValueError or RuntimeError are good
candidates, a custom exception may be as well). This is because we view
assertion errors should indicate a mistake on the part of the person
programming the system (Launchpad in this case) but not a mistake on the
part of the user of the system (the programmer using the web service).

One option would be to decorate exceptions at the point they are raised
with an interface that lazr.restful understands to mean that the
exception should be forwarded to the client as a 400-class HTTP result
as opposed to a 500-class code for undecorated (internal) exceptions.

Straw man:

   raise exported_exception(ProjectNotDeactivatable(
       'This project cannot be deactivated since it is linked to '
       'source packages.'))

Of course, lazr.restful will need to be updated to understand these
decorated exceptions and report the errors to the clients appropriately.

lazr.restfulclient may also need to be updated to understand these new
400 responses.

On the other hand, we may decide on a variant that would let us address
this by only making changes to Launchpad.

We also discussed the fact that architecture changes in the future, such
as lazr.restful supporting/encouraging hand-written view-like adapters
from model to webservice, would provide a mechanism for keeping
webservice concerns out of the model objects, which would be a
preferable division of concerns. The approach we take will depend on
how large an undertaking we want to pursue at the time.

Revision history for this message
Curtis Hovey (sinzui) wrote :

I was hoping for something simple. We know every AssertError is meant to be seen by an engineer. We know for Launchpad the engineer can look at the oops or the log of a test. For external developers we want to adapt the AssertionError to a Response that equates/creates a HTTP 500 and the assertion message is included. I have no Idea what we are trying to adapt to in this case, maybe a LaunchpadBrowserResponse

   <adapter
       for="AssertionError
            canonical.launchpad.webapp.servers.WebServiceClientRequest"
       provides="canonical.launchpad.webapp.servers.LaunchpadBrowserResponse"
       factory="lazr.restful.errors.assertion_error_to_http_500"
       />

Revision history for this message
Benji York (benji) wrote :

> I was hoping for something simple. We know every AssertError is meant
> to be seen by an engineer. We know for Launchpad the engineer can look
> at the oops or the log of a test. For external developers we want to
> adapt the AssertionError to a Response that equates/creates a HTTP 500
> and the assertion message is included.

I think that last sentence is where we diverge. Assertions are for the
people developing the code, not using it. Any time an assertion fires
it indicates a mistake on the part of the programmer(s) that put it in,
not the user (even if the user is another programmer on the other side
of an API).

In the case of OOPS-1694EB1486 the exception generated should not have
been an assertion error because there was no mistake on the part of the
LP developer. The mistake was made by the caller of the function
because that function can not be called on a project that is linked to
source packages.

Similarly, the exception in OOPS-1318S626 should not be an
AssertionError because there was no mistake on the part of the LP
programmer, but on the part of the API user.

I'll change both the assertions in question to instead raise exceptions
(probably ValueError or RuntimeError) with an interface added (via a
wrapper as described in comment #7) that lazr.restful knows to translate
into an HTTP error.

Revision history for this message
Robert Collins (lifeless) wrote : Re: [Bug 413174] Re: API AssertionError could be adapted to a http code

I think there is an undisclosed tension here.

In the past, once things got past the HTTP parser, all API misuses
were *LP developer bugs*.

Now though, folk can use the model code directly via the LP API, and
so things that previously were a programmer error on the LP team are
now a programmer error on a consumer of the API.

It makes sense to me that these are *still AssertionErrors*, its still
a programming error.

(That said, I try not to use assert much if at all, ValueError is a
better way of signalling an incorrect input to a function) - I'm
simply noting that all that has happened here is the population of
'programmers' has increased, whether something is a programmer error
not not hasn't altered.

Revision history for this message
Gary Poster (gary) wrote :

We agree on the tension.

However, I disagree that an assertion error is the kind of error that is appropriate to send to end users. I think assertion errors are for cases that should never happen according to the expected logic of your code block, and as such are always something for Launchpad coders to address. In contrast, many (but, I'm pretty sure, not all) errors are appropriate to share with programmers-over-the-wire, because they indicate things they need to change in their usage of the API.

In regards to the use of asserts, I'm hoping we can agree to disagree for this particular case.

We intend to close this bug once it is possible to explicitly mark a raised error as safe for exporting (its type and value). Then we will convert the two pertinent errors from the OOPS reports to more description exception types (e.g., ValueError) and mark them safe for export. Then we will close the bug.

In my mind, the primary point of disagreement as to what to do now is whether we should have a whitelist or a blacklist of exposing error messages to API users. Whitelists are easier to do safely up-front, obviously, and can be changed to blacklists later if they are determined to be impractical.

Benji is proceeding with the change that he's described now, in our hopes that it will be a good step forward.

Revision history for this message
Robert Collins (lifeless) wrote :

Aesthetically, i'd rather not be raising assertion error anywhere
outside of tests.

However, I'm trying to raise a pragmatic issue here: by not treating
this as a goalposts-have-moved scenario, it places a burden on the LP
team to audit and convert all callsites that are:
 - reachable via the API
 - and can be triggered if the API client sends bogus combinations

Quick grepping suggests 700 or so potential call sites of this nature.

If we include API users in the set of programmers, we don't need to do
a huge audit, but there is a risk that some assertions may include
private data or something. (That exists for transcribed errors too,
which is why IMO it cancels out).

I think at least in the short term it makes a lot more sense to
*clearly signal an assertion over the wire* than to audit and rewrite
all the callsites.

Revision history for this message
Benji York (benji) wrote :

I verified that the exceptions referenced in this bug report no longer generate OOPSes and instead provide a useful error message to the web service user.

Changed in launchpad-foundations:
status: Triaged → Fix Committed
Changed in launchpad-registry:
status: Triaged → Fix Committed
Changed in launchpad-foundations:
assignee: nobody → Benji York (benji)
tags: added: qa-ok
Revision history for this message
Launchpad QA Bot (lpqabot) wrote : Bug fixed by a commit
Changed in launchpad-foundations:
milestone: none → 10.10
tags: added: qa-needstesting
removed: qa-ok
Benji York (benji)
tags: added: qa-ok
removed: qa-needstesting
Changed in launchpad-foundations:
status: Fix Committed → Fix Released
Changed in launchpad-registry:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers