Consistent 400 Response for Invalid Input in Kolibri Public Content APIs

manzil-infinity180 commented 1 week ago

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[bot] commented 1 week ago

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

manzil-infinity180 commented 1 week ago

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

manzil-infinity180 commented 1 week ago

@jredrejo @ozer550 please review it!

MisRob commented 1 week ago

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

manzil-infinity180 commented 6 days ago

@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 commented 4 days ago

@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 commented 4 days ago

@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)

manzil-infinity180 commented 1 day ago

@ozer550 done , review it

learningequality / kolibri