[NDH-304] Cleanly handle trailing slashes in Django paths

Author: abachman-dsac

backend/app/settings.py

@@ -83,6 +83,7 @@
 CORS_ALLOWED_METHODS = ['GET']
 
 ROOT_URLCONF = 'app.urls'
+APPEND_SLASH = True # this is default, but we're making sure it's explicit
 
 TEMPLATES = [
     {

backend/app/tests/__init__.py

@@ -0,0 +1,81 @@
+from django.urls import resolve, reverse
+from django.test import TestCase
+
+
+class ProviderDirectorySpaRouting(TestCase):
+    """
+    Ensure that provider_directory routing is agnostic to paths.
+    """
+
+    def test_homepage_url_resolves_to_correct_view(self):
+        resolver = resolve("/")
+        self.assertEqual(resolver.view_name, "provider_directory:index")
+        self.assertEqual(resolver.func.__name__, "index")
+
+    def test_detail_url_resolves_with_path(self):
+        resolver = resolve("/search/123/")
+        self.assertEqual(resolver.view_name, "provider_directory:index_with_path")
+        self.assertEqual(resolver.kwargs["path"], "search/123/")
+
+    def test_reverse_index_returns_path(self):
+        path = reverse("provider_directory:index")
+        self.assertEqual(path, "/")
+
+    def test_reverse_index_with_path_returns_path(self):
+        path = reverse("provider_directory:index_with_path", kwargs={"path": "search/123"})
+        self.assertEqual(path, "/search/123")
+
+
+class FhirApiRouting(TestCase):
+    """
+    Ensure we maintain a consistent routing configuration, with or without trailing slashes.
+    """
+
+    def assert_valid_routing(self, path_views):
+        for path, expected_view_name in path_views:
+            with self.subTest(f"route {path}"):
+                resolver = resolve(path)
+                self.assertEqual(resolver.view_name, expected_view_name)
+
+    def test_fhir_accepts_optional_trailing_slash(self):
+        path_views = [
+            ("/fhir", "api-root"),
+            ("/fhir/", "api-root"),
+        ]
+        self.assert_valid_routing(path_views)
+
+    def test_fhir_docs_accepts_optional_trailing_slash(self):
+        path_views = [
+            ("/fhir/docs", "schema-swagger-ui"),
+            ("/fhir/docs/", "schema-swagger-ui"),
+            # fallthrough
+            ("/fhirdocs", "provider_directory:index_with_path"),
+            ("/fhirdocs/", "provider_directory:index_with_path"),
+        ]
+        self.assert_valid_routing(path_views)
+
+    def test_fhir_rest_routes_accept_optional_trailing_slash(self):
+        path_views = [
+            # valid paths
+            ("/fhir/Endpoint", "fhir-endpoint-list"),
+            ("/fhir/Endpoint/", "fhir-endpoint-list"),
+            ("/fhir/Endpoint/12345", "fhir-endpoint-detail"),
+            ("/fhir/Endpoint/12345/", "fhir-endpoint-detail"),
+            # fallthrough
+            ("/fhirEndpoint", "provider_directory:index_with_path"),
+            ("/fhirEndpoint/", "provider_directory:index_with_path"),
+            ("/fhirEndpoint/12345", "provider_directory:index_with_path"),
+            ("/fhirEndpoint/12345/", "provider_directory:index_with_path"),
+            ("/fhir/Endpoint12345", "provider_directory:index_with_path"),
+            ("/fhir/Endpoint12345/", "provider_directory:index_with_path"),
+            ("/fhirEndpoint12345", "provider_directory:index_with_path"),
+            ("/fhirEndpoint12345/", "provider_directory:index_with_path"),
+        ]
+        self.assert_valid_routing(path_views)
+
+    def test_fhir_rest_routes_reverse_without_slash(self):
+        endpoint_list_path = reverse("fhir-endpoint-list")
+        self.assertEqual(endpoint_list_path, "/fhir/Endpoint")
+
+        endpoint_detail_path = reverse("fhir-endpoint-detail", kwargs={"pk": 12345})
+        self.assertEqual(endpoint_detail_path, "/fhir/Endpoint/12345")

backend/app/tests/test_routing.py

@@ -18,8 +18,38 @@
 from django.urls import include, path
 from debug_toolbar.toolbar import debug_toolbar_urls
 
+from npdfhir.router import router as npdfhir_router
+
 urlpatterns = [
+    ##
+    # NOTE: (@abachman-dsac) on trailing slashes and routes
+    #
+    # The common django pattern for permitting optional trailing slashes:
+    #
+    #   re_path('fhir/?', ...)
+    #
+    # Allows "/fhirdocs/"" to be a vaild URL, which we do not want.
+    #
+    # Additionally, the use of an empty path string in npdfhir.urls' urlpatterns
+    # `path('', ...)` configuration to mount the rest_framework default router
+    # means that we cannot use a single path matcher with 'fhir/' to mount the
+    # whole npdfhir app and still resolve https://$hostnam/fhir to the
+    # rest_framework.DefaultRouter built-in documentation page.
+    #
+    # By using two path matchers here and pointing the no-trailing-slash path
+    # directly at the rest_framework router api-root view, we can get true
+    # optional trailing slashes.
+    #
+    # The risk for flakiness is due to the fact that `/fhir` is handled by the
+    # path configuration in this file and `/fhir/` is handled by the path
+    # configuration in `npdfhir.urls` at the `path('', ...)` location.
+    #
+    # See app/tests/test_routing.py for validation tests to ensure that changes
+    # inside npdfhir.urls don't break our routing configuration.
     path('fhir/', include("npdfhir.urls")),
+    path('fhir', npdfhir_router.get_api_root_view, name='api-root'),
+    ##
+
     path('admin/', admin.site.urls),
     path('', include('provider_directory.urls')),
 ] + debug_toolbar_urls()

backend/app/urls.py

@@ -0,0 +1,29 @@
+from rest_framework.routers import DefaultRouter
+
+from . import views
+
+
+# NOTE: (@abachman-dsac) even though we're using Django's APPEND_SLASH option,
+# we want to ensure we are liberal in what we accept, and strict in what we
+# generate for API paths.
+#
+# ACCEPT:
+# - /Endpoint
+# - /Endpoint/
+# - /Endpoint/12345
+# - /Endpoint/12345/
+#
+# PRODUCE:
+# - /Endpoint
+# - /Endpoint/12345
+#
+class OptionalSlashRouter(DefaultRouter):
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.trailing_slash = "/?"
+
+
+router = OptionalSlashRouter()
+router.register(r"Practitioner", views.FHIRPractitionerViewSet, basename="fhir-practitioner")
+router.register(r"Organization", views.FHIROrganizationViewSet, basename="fhir-organization")
+router.register(r"Endpoint", views.FHIREndpointViewSet, basename="fhir-endpoint")

backend/npdfhir/router.py

@@ -1,16 +1,16 @@
-from django.urls import path, include
-from rest_framework.routers import DefaultRouter
+from django.urls import path, include, re_path
 from rest_framework.schemas import get_schema_view
-from rest_framework.renderers import JSONOpenAPIRenderer
-from . import views
 from debug_toolbar.toolbar import debug_toolbar_urls
 from drf_yasg.views import get_schema_view
 from drf_yasg import openapi
 
+from . import views
+from .router import router
+
 schema_view = get_schema_view(
     openapi.Info(
         title="NPD FHIR API",
-        default_version='v1',
+        default_version="v1",
         description="The National Provider Directory FHIR API exposes public information on all Providers and Organizations that have registered through the National Provider and Payer Enumeration System NPPES. This is a limited beta release; coverage and data quality will increase iteratively.",
         terms_of_service="TBD",
         contact=openapi.Contact(email="opensource@cms.hhs.gov"),
@@ -19,22 +19,11 @@
     public=True,
 )
 
-router = DefaultRouter()
-router.register(r'Practitioner', views.FHIRPractitionerViewSet,
-                basename='fhir-practitioner')
-router.register(r'Organization', views.FHIROrganizationViewSet,
-                basename='fhir-organization')
-router.register(r'Endpoint', views.FHIREndpointViewSet,
-                basename='fhir-endpoint')
-
 urlpatterns = [
-    path('docs.<format>/',
-         schema_view.without_ui(cache_timeout=0), name='schema-json'),
-    path('docs/', schema_view.with_ui('swagger',
-         cache_timeout=0), name='schema-swagger-ui'),
+    path("docs.<format>/", schema_view.without_ui(cache_timeout=0), name="schema-json"),
+    re_path("docs/?", schema_view.with_ui("swagger", cache_timeout=0), name="schema-swagger-ui"),
     path("healthCheck", views.health, name="healthCheck"),
     # path('metadata', views.fhir_metadata, name='fhir-metadata'),
-
-    # Router URLs
-    path('', include(router.urls), name='index')
+    # everything else is passed to the rest_framework router to manage
+    path("", include(router.urls), name="index"),
 ] + debug_toolbar_urls()

backend/npdfhir/urls.py

@@ -6,7 +6,8 @@
 from django.urls import reverse
 
 TEST_DIR = Path(os.path.dirname(os.path.abspath(__file__)))
-STATIC_INDEX = TEST_DIR / '..' / 'static' / 'index.html'
+STATIC_INDEX = TEST_DIR / ".." / "static" / "index.html"
+
 
 class WithoutStaticIndex(TestCase):
     """
@@ -22,8 +23,9 @@ def test_index_redirects_to_vite_in_development(self):
         """
         When static/index.html doesn't exist, route redirects
         """
-        response = self.client.get(reverse('index'))
-        self.assertRedirects(response, 'http://localhost:3000/', fetch_redirect_response=False)
+        response = self.client.get(reverse("provider_directory:index"))
+        self.assertRedirects(response, "http://localhost:3000/", fetch_redirect_response=False)
+
 
 class WithStaticIndex(TestCase):
     """
@@ -33,12 +35,12 @@ class WithStaticIndex(TestCase):
     @classmethod
     def setUpTestData(cls):
         if not os.path.exists(STATIC_INDEX):
-            with open(STATIC_INDEX, 'a') as index:
-                index.write(f'\n<!-- test content {datetime.now()} -->')
+            with open(STATIC_INDEX, "a") as index:
+                index.write(f"\n<!-- test content {datetime.now()} -->")
 
     def test_index_serves_static_file(self):
         """
         When static/index.html exists, route serves it
         """
-        response = self.client.get(reverse('index'))
-        self.assertContains(response, 'test content', status_code=HTTPStatus.OK)
+        response = self.client.get(reverse("provider_directory:index"))
+        self.assertContains(response, "test content", status_code=HTTPStatus.OK)

backend/provider_directory/tests/test_views.py

@@ -2,7 +2,8 @@
 from . import views
 from debug_toolbar.toolbar import debug_toolbar_urls
 
+app_name = "provider_directory"
 urlpatterns = [
-    path(r'', views.index, name='index'),
-    path(r'<path:path>', views.index),
-]
+    path(r"", views.index, name="index"),
+    path(r"<path:path>", views.index, name="index_with_path"),
+]

backend/provider_directory/urls.py

@@ -0,0 +1,6 @@
+line-length = 100
+
+[format]
+quote-style = "double"
+indent-style = "space"
+docstring-code-format = true