SQL errors should not cause Internal Server Error

[ktal90]

Hi all,

I have the models and relationships as shown below. As you can see, the parent_id column is a non-nullable ForeignKey. When I attempt to delete a parent for one of these children, the response is a 500 Internal Server Error with HTML that includes the actual SQL error.

For proper API behavior, an underlying SQL error should produce a 4xx error that returns the SQL error the same way that a validation error would be produced by Eve, i.e. with the _error property.

class CommonColumns(Base):
    __abstract__ = True
    _created = Column(DateTime, default=func.now())
    _updated = Column(DateTime, default=func.now(), onupdate=func.now())
    _etag = Column(String(40))


class Child(CommonColumns):
    __tablename__ = 'children'
    id = Column(Integer, primary_key=True, autoincrement=True)
    title = Column(String(128))
    parent_id = Column(Integer, ForeignKey('parents.id'), nullable=False)
    parent = relationship('Parents', back_populates='children')


class Parents(CommonColumns):
    __tablename__ = 'parents'
    id = Column(Integer, primary_key=True, autoincrement=True)
    title = Column(String(128))
    children = relationship('Child', back_populates='parent')

To implement this, I believe the sqlalchemy.exc.IntegrityError should be caught by Eve-SQLAlchemy and wrapped up in an Eve error response format. I'm happy to take this, if the approach is valid.

Edit: To clarify, the SQL error is expected. It is caused by the cascade default when dropping foreign keys. This shouldn't be an Internal Server Error, though. The fault should be signaled to the client.

[dkellner]

Sorry for the late answer.

I agree that in cases it's the client's fault, we should use 4xx error codes. But some SQL errors will really be internal and the client cannot do anything about that - those should stay 5xx errors in my opinion.

[ktal90]

Thanks for getting back @dkellner . I agree that some will be true errors, and we shouldn't blanket catch and return a 4xx. However, for a sqlalchemy.exc.IntegrityError exception, that is clearly the client (or API, I suppose) offending the underlying database. In my example where a relationship shouldn't be broken because of the non-nullable ForeignKey, that is actually a bad request by the client. A 4xx making this clear to the client should be the expected behavior. IMO, this is actually similar to any non-nullable constraint. However, Eve provides the validation to make a field non-nullable at the API layer. It does not do this for relationships, as far as I know. What do you think?

[dkellner]

Yes, I totally agree. I just wanted to make sure we're still making a distinction between errors the user/client can actually handle to the ones outside of its responsibility.

Catching the exception and handling it sounds like a good approach to me.

Do you have time to give this a shot?