Consistent 400 Response for Invalid Input in Kolibri Public Content APIs

[manzil-infinity180]

Summary

This pr is intended to be consistent for 400 response for retrieve function of ContentNodeTreeViewset and ImportMetadataViewset invalid input by returning a 404 response.

References

Closes: #12807

Reviewer guidance

…

---

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical user journeys are covered by Gherkin stories
• Critical and brittle code paths are covered by unit tests

PR process

• PR has the correct target branch and milestone
• PR has 'needs review' or 'work-in-progress' label
• If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
• If this is an important user-facing change, PR or related issue has a 'changelog' label
• If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

• PR is fully functional
• PR has been tested for accessibility regressions
• External dependency files were updated if necessary (yarn and pip)
• Documentation is updated
• Contributor is in AUTHORS.md

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0.dev0_git.20241119191753.pex
Windows Installer (EXE)	kolibri-0.18.0.dev0+git.20241119191753-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0.dev0+git.20241119191753-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0.dev0+git.20241119191753.dmg
Android Package (APK)	kolibri-0.18.0.dev0+git.20241119191753-0.1.4-debug.apk
TAR file	kolibri-0.18.0.dev0+git.20241119191753.tar.gz
WHL file	kolibri-0.18.0.dev0+git.20241119191753-py2.py3-none-any.whl

[jredrejo]

This is totally valid from my point of view, pk being None return a 400 error code, while not finding a pk should return a 404 code.
This pr solves first case.
However, tests must be fixed to pass

tests are not passing

[jredrejo]

Tests are not passing because pk is being provided, for this test to pass you either must set pk to none or change the code to return 400 when pk is not an UUID (which current code is not doing)

self = <kolibri.core.content.test.test_content_app.ContentNodeAPITestCase testMethod=test_contentnode_tree_bad_pk>

    def test_contentnode_tree_bad_pk(self):
        response = self.client.get(
            reverse(
                "kolibri:core:contentnode_tree-detail",
                kwargs={"pk": "this is not a UUID"},
            )
        )
>       self.assertEqual(response.status_code, 400)
E       AssertionError: 404 != 400

[manzil-infinity180]

Tests are not passing because pk is being provided, for this test to pass you either must set pk to none or change the code to return 400 when pk is not an UUID (which current code is not doing)

self = <kolibri.core.content.test.test_content_app.ContentNodeAPITestCase testMethod=test_contentnode_tree_bad_pk>

    def test_contentnode_tree_bad_pk(self):
        response = self.client.get(
            reverse(
                "kolibri:core:contentnode_tree-detail",
                kwargs={"pk": "this is not a UUID"},
            )
        )
>       self.assertEqual(response.status_code, 400)
E       AssertionError: 404 != 400

I tried with pk as None but it gives me 404 error

def test_import_metadata_bad_pk(self):
        response = self.client.get(
            reverse(
                "kolibri:core:importmetadata-detail",
                kwargs={"pk": None},
            )
        )
>       self.assertEqual(response.status_code, 400)
E       AssertionError: 404 != 400

kolibri/core/content/test/test_public_api.py:105: AssertionError
----------------------------------------------------------------------------- Captured stderr setup -----------------------------------------------------------------------------
ERROR    2024-11-13 01:19:35,987 No tree cache found for 3 levels and 5 children per level
------------------------------------------------------------------------------ Captured log setup -------------------------------------------------------------------------------
ERROR    kolibri.core.content.test.test_channel_upgrade:test_channel_upgrade.py:110 No tree cache found for 3 levels and 5 children per level
----------------------------------------------------------------------------- Captured stdout call ------------------------------------------------------------------------------
INFO     2024-11-13 01:19:36,823 127.0.0.1 - - "GET /api/public/v2/importmetadata/None/" 404 0 "" "unknown"
WARNING  2024-11-13 01:19:36,824 Not Found: /api/public/v2/importmetadata/None/
------------------------------------------------------------------------------- Captured log call -------------------------------------------------------------------------------
WARNING  django.request:log.py:224 Not Found: /api/public/v2/importmetadata/None/
=============================================================================== warnings summary ================================================================================

but changing into the retrieve function as pk == "this is not a UUID" it worked(gives 400 response) only for ImportMetadataViewset

[ozer550]

Requested some changes involving throwing a 404 when the pk is None. I would also suggest to change the get_object call to get_object_or_404 to maintain consistency with studio as commented here by @rtibbles . I think @jredrejo also agrees with this?

[manzil-infinity180]

@jredrejo @ozer550 please review it!

[MisRob]

Thanks all and @manzil-infinity180 welcome and thanks for working on what seems to be your first contribution to Kolibri!

[ozer550]

A little more clarity on some of the thoughts mentioned earlier. Feel free to ask any more questions to clarify!

[ozer550]

Hi @manzil-infinity180! We still want this functionality of checking if the pk is a valid UUID or not. We want both of the checks that if the UUID is invalid then we throw a 400 and if it is None then we would raise 404. so both the checks should simultaneously exist.

[manzil-infinity180]

@ozer550 Hello, i implemented as you intended above
But checking with the UUID i am getting 400 response code even if i pass pk as NONE or invalid UUID as pk (ImportMetadataViewset)
and 404 for both pk as None and invalid uuid (ContentNodeViewset)
Let me know is i am doing any thing wrong?


If i can replace None with some valid UUID but not content node in test like this then only it will hit this get_object_or_404 ( i guess UUID taking None as invalid UUID)

def test_import_metadata_pk(self):
        response = self.client.get(
            reverse(
                "kolibri:core:importmetadata-detail",
                kwargs={"pk": "20f5484b88ae49b08af03a389b4917dd"},
            )
        )
        self.assertEqual(response.status_code, 404)

[ozer550]

@ozer550 Hello, i implemented as you intended above But checking with the UUID i am getting 400 response code even if i pass pk as NONE or invalid UUID as pk (ImportMetadataViewset) and 404 for both pk as None and invalid uuid (ContentNodeViewset) Let me know is i am doing any thing wrong? If i can replace None with some valid UUID but not content node in test like this then only it will hit this get_object_or_404 ( i guess UUID taking None as invalid UUID)

def test_import_metadata_pk(self):
        response = self.client.get(
            reverse(
                "kolibri:core:importmetadata-detail",
                kwargs={"pk": "20f5484b88ae49b08af03a389b4917dd"},
            )
        )
        self.assertEqual(response.status_code, 404)

So the current tests that you have implemented are failing?

[manzil-infinity180]

@ozer550 Hello, i implemented as you intended above But checking with the UUID i am getting 400 response code even if i pass pk as NONE or invalid UUID as pk (ImportMetadataViewset) and 404 for both pk as None and invalid uuid (ContentNodeViewset) Let me know is i am doing any thing wrong? If i can replace None with some valid UUID but not content node in test like this then only it will hit this get_object_or_404 ( i guess UUID taking None as invalid UUID)

def test_import_metadata_pk(self):
        response = self.client.get(
            reverse(
                "kolibri:core:importmetadata-detail",
                kwargs={"pk": "20f5484b88ae49b08af03a389b4917dd"},
            )
        )
        self.assertEqual(response.status_code, 404)

So the current tests that you have implemented are failing?

Yeah only two test passing, for when pk is none (Content node Viewset) and when pk is invalid (ImportMetadataViewset)

[ozer550]

Couple of corrections from myside over things that I overlooked! we are almost there.

[manzil-infinity180]

@ozer550 done , review it

[ozer550]

Great work @manzil-infinity180! I know we iterated quite a lot of times on this but we both learnt new things. Congratulations on your first contribution to kolibri. Looking forward towards more!

[MisRob]

@jredrejo was all your feedback addressed and do you feel good about merging?

[m3tal10]

Looks great. This ensures that if the PK is not available then the response must raise a 404(not found) as per requirements and if the pk is not a valid UUID then it should raise a ValueError 400(bad request). The tests also ensure the validity of the response perfectly.

[jredrejo]

LGTM