API AssertionError could be adapted to a http code

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.

The adaptation makes sense to me.

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?

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.

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.

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

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.

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"
       />

> 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.

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.

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.

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.

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.

Fixed in stable r11548 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/11548>.