LF-4092 - update animal controllers to use handle objection error #3148

Duncan-Brain · 2024-03-01T23:15:26Z

Description

On creation of the objection error handler it was mentioned by @SayakaOno that we should update all animal controllers to be consistent. (and a modest -50 lines of code ;) )

Now every endpoint in animals with a trx and excess checks should be covered.

Also added in a test that was missing from batch and controller -- should not be able to add a default breed with a custom type.

To test: For exhaustive testing remove one check at a time and check that it is still handled.

Jira link: LF-4092

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Passes test case
UI components visually reviewed on desktop view
UI components visually reviewed on mobile view
Other (please explain)

Checklist:

I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
The precommit and linting ran successfully
I have added or updated language tags for text that's part of the UI
I have added "MISSING" for all new language tags to languages I don't speak
I have added the GNU General Public License to all new files

SayakaOno

Thank you for working on this!
I prefer receiving 400 rather than 500 when possible. I would like to hear what others think!

packages/api/src/controllers/customAnimalBreedController.js

SayakaOno · 2024-03-04T21:07:08Z

packages/api/src/controllers/animalController.js

-        if (!Array.isArray(req.body)) {
-          await trx.rollback();
-          return res.status(400).send('Request body should be an array');
-        }


In my humble opinion, this is nicer than an Unknown internal server error. You haven't removed the one from animalBatchController!

Its more explicit yeah, for me I prefer to not feel like I have to handle every case where there should be an array provided since it could add up to a lot of code.

The message provided by objections error "req.body is not iterable" makes enough sense to me. But I also think if you want to make a new Error Type in the future that makes this generic error more readable -- that is nice too!

SayakaOno · 2024-03-04T21:08:05Z

packages/api/src/controllers/animalGroupController.js

-        if (!Array.isArray(related_animal_ids) || !Array.isArray(related_batch_ids)) {
-          await trx.rollback();
-          return res.status(400).send('Animal ids and batch ids must be arrays');
-        }


I prefer keeping this too!

Duncan-Brain · 2024-03-06T21:44:18Z

I agree it is nice to receive 400 where possible. But to me the solution is to create handled error code types. And continue to allow "unknowns" to be 500.

Let the bad 500 be incentive to make more error codes haha

antsgar · 2024-03-07T12:08:08Z

@Duncan-Brain is there a way we could add more cases to the handle error function so that what is currently returned as a 400 would still be returned as a 400?

…ests

Duncan-Brain · 2024-03-07T22:42:38Z

@SayakaOno @antsgar I added a custom test for the Array check .. I don't really love the solution I came up with since it seems a little messy, and the error message is not the nicest of things... but check it out maybe you think it is ok.

One thing that came up from doing this:

In our controllers we use the Models to write to the DB, and in our tests we use plain Knex. By not testing our controller during tests this creates two different error messages. Handled in this case, but reasonable to think we will forget and cause some problems down the line... I think we should be testing our API through our controllers and not knex directly, especially since we have been putting more constraint and response logic in the Model directly -- and thats how we use our api so is a better test for what we expect to happen.

Also there was a test in animal_groups being flakey (the order it was returned was not the same as it was added in test) so I made a little band-aid there.

SayakaOno

I'm open to approval, but I'm still in favour of finding a better way of validating the request body that Duncan will feel comfortable with rather than letting the handleCustomError function handle internal errors :)

packages/api/tests/animal_group.test.js

Duncan-Brain · 2024-03-08T20:49:33Z

I can put all the Array checking back in if that is what is preferred by others too. Maybe we can discuss in tech daily... I definitely agree it is not as nice looking as is.

I think when we add typescript to the backend we could enforce types that way a lot easier and the returned errors would be more parseable or automatic. We just don't have that right now.

For me the reason why I do not personally want to make it nicer now is that:

Error is not translated so it is not really user facing -- and so it is only useful for us the developers.
The burden (lines of code) of enforcing every type, for every variable, on every route seems high and the value of letting a generic error handle it seems useful to me.
I guess I see it that creating a custom error message for related_animal_ids = ARRAY is no more important than creating a custom error message for breed = STRING (putting "breed": 5 in your body will throw an objection error that unless you know what it means it might not make sense either).

antsgar · 2024-03-12T19:03:10Z

packages/api/src/util/errorCodes.js

+const isNotIterable = 'is not iterable';
+// Plain knex response for tests
+const isNotIterableTests =
+  'Invalid attempt to iterate non-iterable instance.\nIn order to be iterable, non-array objects must have a [Symbol.iterator]() method.';


I'm not a fan of this parsing of specific error messages tbh, it seems brittle -- if they change the message in the slightest this will break. Out of all the options, I think I'd rather just keep the 500 or keep the explicit checks for 400s. Thoughts?

I am personally for keeping the 500 until we add typescript to backend or some other request shape checker.

I'm okay with that, prefer it to this last approach!

Last approach it is! lol

@antsgar should I make a tech debt ticket (spike, rfc) about specifying our backend request shape? There has been a few suggestions in the past about improving our backend in this regard and we have OAS stuff but to my knowledge we haven't chosen a direction for this improvement yet.

This reverts commit 8911186.

…s, fix tests" This reverts commit 33de7bf.

…kiness fix

LF-4092 - update animal controllers to use handle objection error

9acae09

Duncan-Brain requested review from a team as code owners March 1, 2024 23:15

Duncan-Brain requested review from antsgar and SayakaOno and removed request for a team March 1, 2024 23:15

LF-4092 - fix tests

edb41c4

Duncan-Brain self-assigned this Mar 4, 2024

SayakaOno reviewed Mar 4, 2024

View reviewed changes

LF-4092 - remove commented code

a848476

LF-4092 - Add support for custom error codes, check for arrays, fix t…

33de7bf

…ests

SayakaOno reviewed Mar 8, 2024

View reviewed changes

packages/api/tests/animal_group.test.js Outdated Show resolved Hide resolved

LF-4092 - rename patch on test

8911186

SayakaOno previously approved these changes Mar 8, 2024

View reviewed changes

antsgar reviewed Mar 12, 2024

View reviewed changes

Duncan-Brain added 3 commits March 13, 2024 18:21

Revert "LF-4092 - rename patch on test"

ad7e4c7

This reverts commit 8911186.

Revert "LF-4092 - Add support for custom error codes, check for array…

35d9851

…s, fix tests" This reverts commit 33de7bf.

LF-4092 - revert custom error handler changes but keep group test fla…

638e5cc

…kiness fix

Duncan-Brain dismissed SayakaOno’s stale review via 638e5cc March 13, 2024 22:27

Duncan-Brain requested review from antsgar and SayakaOno March 13, 2024 22:31

SayakaOno approved these changes Mar 15, 2024

View reviewed changes

antsgar approved these changes Mar 18, 2024

View reviewed changes

antsgar added this pull request to the merge queue Mar 18, 2024

Merged via the queue into integration with commit 6df88d4 Mar 18, 2024
5 checks passed

antsgar deleted the LF-4092-add-error-handler-to-animal-endpoints branch March 18, 2024 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LF-4092 - update animal controllers to use handle objection error #3148

LF-4092 - update animal controllers to use handle objection error #3148

Duncan-Brain commented Mar 1, 2024 •

edited

Loading

SayakaOno left a comment

SayakaOno Mar 4, 2024

Duncan-Brain Mar 6, 2024 •

edited

Loading

SayakaOno Mar 4, 2024

Duncan-Brain commented Mar 6, 2024

antsgar commented Mar 7, 2024

Duncan-Brain commented Mar 7, 2024 •

edited

Loading

SayakaOno left a comment

Duncan-Brain commented Mar 8, 2024

antsgar Mar 12, 2024

Duncan-Brain Mar 12, 2024

antsgar Mar 13, 2024

Duncan-Brain Mar 13, 2024 •

edited

Loading

LF-4092 - update animal controllers to use handle objection error #3148

LF-4092 - update animal controllers to use handle objection error #3148

Conversation

Duncan-Brain commented Mar 1, 2024 • edited Loading

SayakaOno left a comment

Choose a reason for hiding this comment

SayakaOno Mar 4, 2024

Choose a reason for hiding this comment

Duncan-Brain Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

SayakaOno Mar 4, 2024

Choose a reason for hiding this comment

Duncan-Brain commented Mar 6, 2024

antsgar commented Mar 7, 2024

Duncan-Brain commented Mar 7, 2024 • edited Loading

SayakaOno left a comment

Choose a reason for hiding this comment

Duncan-Brain commented Mar 8, 2024

antsgar Mar 12, 2024

Choose a reason for hiding this comment

Duncan-Brain Mar 12, 2024

Choose a reason for hiding this comment

antsgar Mar 13, 2024

Choose a reason for hiding this comment

Duncan-Brain Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Duncan-Brain commented Mar 1, 2024 •

edited

Loading

Duncan-Brain Mar 6, 2024 •

edited

Loading

Duncan-Brain commented Mar 7, 2024 •

edited

Loading

Duncan-Brain Mar 13, 2024 •

edited

Loading