Fix CORS preflight handling#17
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds explicit CORS support to NodeNormalization’s Tornado handlers so browser-based clients can successfully perform CORS preflight (OPTIONS) requests (fixes #13), and adds a targeted regression test suite.
Changes:
- Introduces
NodeNormalizationBaseHandlerwith CORS header handling + anoptions()implementation for preflight requests. - Updates API endpoint handlers to inherit from
NodeNormalizationBaseHandlerso CORS applies across endpoints. - Adds
tests/test_cors.pyand hardensNodeNormalizationAPINamespace.load_configuration()to tolerate missing CLI options (viagetattr).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_cors.py | New tests validating CORS headers on GET and OPTIONS (preflight). |
| src/nodenorm/handlers/base.py | New shared handler implementing CORS headers + OPTIONS handling. |
| src/nodenorm/handlers/version.py | Switches handler base class to include CORS behavior. |
| src/nodenorm/handlers/set_identifiers.py | Switches handler base class to include CORS behavior. |
| src/nodenorm/handlers/semantic_types.py | Switches handler base class to include CORS behavior. |
| src/nodenorm/handlers/normalized_nodes.py | Switches handler base class to include CORS behavior. |
| src/nodenorm/handlers/health.py | Switches handler base class to include CORS behavior. |
| src/nodenorm/handlers/conflations.py | Switches handler base class to include CORS behavior. |
| src/nodenorm/handlers/curie_prefix.py | Switches handler base class to include CORS behavior. |
| src/nodenorm/namespace.py | Makes option overrides tolerant of missing conf/host/port attributes. |
Comments suppressed due to low confidence (1)
src/nodenorm/handlers/curie_prefix.py:15
curie_prefix.pydefinesclass SemanticTypeHandlerand constructs a response undersemantic_types, which conflicts with the module/endpoint intent (/get_curie_prefixes,name = "curie_prefixes"). Since this class was modified in this PR (base class swap), it’s a good opportunity to rename it (and its response variable/shape if needed) to avoid confusion and future misrouting when/if it gets wired intobuild_handlers().
class SemanticTypeHandler(NodeNormalizationBaseHandler):
"""
Mirror implementation to the renci implementation found at
https://nodenormalization-sri.renci.org/docs
We intend to mirror the /get_curie_prefixes endpoint
"""
name = "curie_prefixes"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+12
to
+24
| def set_default_headers(self): | ||
| origin = self.request.headers.get("Origin") | ||
| if origin is None: | ||
| return | ||
|
|
||
| requested_headers = self.request.headers.get("Access-Control-Request-Headers") | ||
|
|
||
| self.set_header("Access-Control-Allow-Origin", origin) | ||
| self.set_header("Access-Control-Allow-Credentials", "true") | ||
| self.set_header("Access-Control-Allow-Methods", self.cors_methods) | ||
| self.set_header("Access-Control-Allow-Headers", requested_headers or "*") | ||
| self.set_header("Access-Control-Max-Age", self.cors_max_age) | ||
| self.set_header("Vary", "Origin") |
Comment on lines
+1
to
+21
| import importlib.util | ||
| from pathlib import Path | ||
|
|
||
| import tornado.web | ||
| from tornado.testing import AsyncHTTPTestCase | ||
|
|
||
|
|
||
| ORIGIN = "https://translatorsri.github.io" | ||
| BASE_HANDLER_PATH = Path(__file__).parents[1] / "src" / "nodenorm" / "handlers" / "base.py" | ||
|
|
||
|
|
||
| def load_base_handler(): | ||
| spec = importlib.util.spec_from_file_location("_nodenorm_base_handler_under_test", BASE_HANDLER_PATH) | ||
| module = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(module) | ||
| return module.NodeNormalizationBaseHandler | ||
|
|
||
|
|
||
| NodeNormalizationBaseHandler = load_base_handler() | ||
|
|
||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/nodenorm/handlers/curie_prefix.py:11
- In
curie_prefix.py, the handler class is namedSemanticTypeHandler, which duplicates the name used insemantic_types.pyand doesn’t match the module/docstring intent of handling/get_curie_prefixes. This is confusing and makes it easy to import the wrong handler; renaming the class (and aligning the response payload/name to the curie-prefixes endpoint) would make the codebase clearer.
class SemanticTypeHandler(NodeNormalizationBaseHandler):
"""
Mirror implementation to the renci implementation found at
https://nodenormalization-sri.renci.org/docs
Comment on lines
+21
to
+23
| self.set_header("Access-Control-Allow-Origin", self.cors_origin) | ||
| self.set_header("Access-Control-Allow-Credentials", self.cors_allow_credentials) | ||
| self.set_header("Access-Control-Allow-Methods", self.cors_methods) |
Comment on lines
4
to
8
| from nodenorm.handlers.base import NodeNormalizationBaseHandler | ||
|
|
||
|
|
||
| class SemanticTypeHandler(BaseHandler): | ||
| class SemanticTypeHandler(NodeNormalizationBaseHandler): | ||
| """ |
| cors_methods = "GET, POST, HEAD, OPTIONS" | ||
| cors_max_age = "600" | ||
|
|
||
| def set_default_headers(self): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #13