fix: no empty transaction on bad URL/method #2365
Conversation
Just convert them to MTOther instead
steve-chavez
force-pushed
the
fix-empty-tx
branch
from
23c45d7 to
4870f94
Compare
| -- TODO is this right? When no tbl is found on the schema cache we disallow OPTIONS? | ||
| throwError $ Error.ApiRequestError ApiRequestTypes.NotFound |
There was a problem hiding this comment.
@laurenceisla Was this done on purpose?
There was a problem hiding this comment.
I think that's the intended response: 404 on a non existent resource. Also, this line was added long ago, according to the blame history.
Ah, I think I get it now. So, if the schema cache is not stale and there is no table, then a 404 response is OK. But if the schema is stale and no table is found as a consequence, then a 404 response is wrong, because doing a GET to the endpoint, for instance, should work. I think there's nothing much it can be done in that case if we don't want to hit the database on every OPTIONS request.
There was a problem hiding this comment.
Agree. So on a stale schema cache for a table, OPTIONS should respond with the possible methods - GET,POST,PATCH, DELETE but no PUT since it requires its PK inside the schema cache.
There was a problem hiding this comment.
Hm, wait. We talked about this on #1839 (comment). Of course GET/POST should be allowed but now that we restrict PATCH/DELETE, we cannot include them in the allow unless they include a filter.
This would be better left for another PR.
steve-chavez
force-pushed
the
fix-empty-tx
branch
from
4870f94 to
efaa84e
Compare
|
Will fix codecov failures in another PR. It would be too many changes doing it here. |
Closes #2364.
Removes the
ActionUnknownandTargetUnknowntypes and ensures an error is raised at the ApiRequest level(no db connection from the pool here).Also untangles the
ApiRequestcode a bit, making it more linear.Not using connections cannot be tested for now but once metrics(#2129) is merged it could be done by checking the pool
takes.