Repository navigation
Release v3.1.0 - #37
Merged
Merged
Conversation
For SQLite configs, connectInfo['tap_schema'] is the FILE PATH (passed to ATTACH DATABASE), not the schema name. The schema name used in queries is held in connectInfo['tap_schema_file'] (set as ATTACH AS in tapquery.py). TableValidator and dataDictionary both used connectInfo['tap_schema'] as the schema name when constructing SQL like: SELECT table_name FROM <schema>.tables which produced "near "/": syntax error" against SQLite because the generated SQL embedded the file path. Surfaced by the new CI SQLite fixture; would have prevented any SQLite- backed production deployment from passing table validation. Oracle/ Postgres/MySQL behavior is unchanged — those configs don't set tap_schema_file, so the existing tap_schema lookup remains the fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 1+2 of the CI plan. Establishes a GitHub Actions gate that runs on every PR and on push to develop/main, replacing the manual KOA/NEID/NEA testing loop with automated checks. What runs in CI: - ruff (E, F, W, I) — gated; codebase passes with the minimal ignore set documented in pyproject.toml - pytest tests/ — matrix over Python 3.9 / 3.10 / 3.11 / 3.12 on Ubuntu - python -m build --wheel — Ubuntu + macOS, verifies the C extension builds cross-platform and the wheel is installable Test infrastructure: - tests/conftest.py spins up an HTTP server on a free port backed by a custom nph-aware request handler (stdlib's CGIHTTPRequestHandler prepends its own status line, breaking nph- scripts that emit their own HTTP/1.1 status lines like nph-tap.py does) - tests/fixtures/build_test_db.py builds tap_schema.db (IVOA-shape) and test_data.db (3 public + 3 expired + 4 still-embargoed L1 rows). The still-embargoed rows are the key fixture: production test servers (vmneidtest, koa-test) have all-expired propints, which means propflag enforcement can't be observed there. CI now has the missing scenario. - tests/fixtures/stubs/spatial_index.py is a minimal stub for the four module-level constants TAP imports. The real spatial_index package on PyPI only ships wheels for Python ≤ 3.8, so modern-Python CI cannot install it. Tracking as upstream issue. - tests/test_http_endpoint.py is a smoke layer that exercises the full HTTP→CGI→TAP→SQLite→IPAC pipeline. Security/regression tests land in the follow-up PR (Phase 3+4). Cosmetic ruff autofixes touched 13 files (whitespace, f-strings, import order) — no semantic changes. Deferred to follow-up PRs: - F821 ignored in ruff for now; 6 pre-existing undefined-name bugs to address in TAP/taputil.py (SQLite/MySQL connection branches) and TAP/tap.py:2402 (debug-log typo `result` → `results`) - spatial_index Python 3.9+ wheel situation upstream - Postgres/MySQL/Oracle CI matrix (SQLite-only for first cut) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…index The wheel-build verification and the test-job install both failed on the first CI run because pip can't resolve spatial_index on Python 3.9+: the PyPI package only ships wheels for 2.7 / 3.6 / 3.7 / 3.8 and has no source distribution. Workaround in CI: - Test job: install nexsciTAP and ADQL with --no-deps. The test fixture stub in tests/fixtures/stubs/spatial_index.py supplies the four module-level constants TAP uses at import time. configobj (the only other actually-needed runtime dep) is in requirements-test.txt. - Wheel-platforms job: drop the post-build pip install; instead verify the .so artifact is present in the wheel. The wheel itself builds fine — the issue is only at install-resolve time. This is purely a CI workaround. The deeper fix is upstream in spatial_index (publish modern wheels or an sdist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Surfaced by the second CI run: TAP/tablenames.py imports sqlparse, TAP/datadictionary.py imports astropy.io.ascii, and other TAP modules import bs4 and xmltodict. None of these are declared in setup.py's install_requires (a pre-existing bug — declaring them in install_requires is a separate cleanup since the CI install path uses --no-deps anyway). Add them to requirements-test.txt so CI installs the actual runtime surface needed to run the tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 3+4 of the CI plan. Each test in test_security_e2e.py is named after the specific regression or security boundary it guards, so any future change that re-opens one of those holes turns the corresponding test red at PR time. What's tested (12 active, 1 skipped): - Table whitelist: rejects sqlite_master, rejects unregistered tables, allows propfilter internal tables (test_access) in subqueries - Table extractor: ORDER BY not misidentified, UNKNOWN-statement-skip bypass closed (Oracle ROWNUM) - propflag: zero honored on L0, complex CASE WHEN with subquery FROM does not misfire the whitelist - Statement validator: DML/DDL rejected, dangerous Oracle functions rejected, semicolon-chained statements rejected - HTTP status codes: 400 for query errors, error bodies don't leak internal state (Traceback, paths, Password=) Skipped with detailed reasons in the test bodies: - test_propflag_bypass_routes_l1_through_propfilter — configparam.py only reads PROPFILTER/ACCESS_TBL/USERS_TBL inside its `if dbms == 'oracle'` branch, so on SQLite the propfilter config is silently dropped and the guard becomes a no-op. End-to-end propflag coverage needs the Oracle/Postgres CI matrix from the deferred plan work. pyNEID compat (test_pyneid_compat.py) — module-level skip. pyNEID's query_adql uses the async TAP endpoint (POST + status URL polling + results fetch); the test fixture handler doesn't yet model the per-request workspace lifecycle Apache provides in production. Direct-HTTP coverage of the same code paths is in test_security_e2e.py. Conftest: handler now routes both /cgi-bin/TAP/nph-tap.py/<endpoint> (direct CGI URL) and /TAP/<endpoint> (production short form) to the same nph-tap.py script — wires up the URL shape pyNEID expects, even though pyNEID itself remains skipped pending async work. Misalignment surfaced: TAP returns 400 for DML / dangerous-function / semicolon-chained rejections (validate_statement raises plain Exception), but 403 for system-catalog rejections (raises TableValidationError). These are all security-policy decisions and should arguably all be 403. Tests match current behavior; harmonizing is a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add CI: GitHub Actions, SQLite fixture, ruff lint
Add security regression tests + pyNEID compat scaffolding
Brings the vosi-tables-shortcut fix (PR #22) and v3.0.1 version bumps from main into develop so the next develop -> main release merge doesn't revert them. # Conflicts: # TAP/tap.py # setup.py
Merge main into develop: catch up v3.0.1 hotfix
Both VOSI endpoints emitted their XML document with no HTTP status line and no headers at all -- the first bytes on the wire were `<?xml version="1.0"...`. These are nph- (non-parsed-headers) CGI endpoints, so the script owns the entire response; nginx and Cloudflare reject the result as a bad upstream response. Every other response in tap.py already prints a CRLF-terminated status line and headers; these two were simply missing them. Emit `HTTP/1.1 200 OK`, `Content-type: text/xml` and the blank header-terminating line, all CRLF-terminated, matching the existing convention in this file (print supplies the LF after the literal \r). Also dispatch these two endpoints before workspace resolution. They are static documents that need no workspace, but they were falling through to the "retrieve workspace from jobid" branch with an empty jobid. That resolved to <workdir>/TAP and returned a 500 whenever that directory did not already exist -- so on a fresh deployment these endpoints failed outright, and on an established one they reached the emitter and produced the header-less response above. Add tests/test_vosi_http_response.py, which asserts on raw socket bytes rather than going through a forgiving client library: a CRLF-terminated status line, no bare LF or CR within any header line, CRLFCRLF ending the header block, an XML Content-type, and an intact body. Verified red/green both ways -- the tests fail against the unpatched code and also catch the `HTTP/1.1 200 OK\n` bare-LF variant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ruff check .` has been failing on develop since PR #23 merged (2026-06-02), on a W291 trailing-whitespace hit at TAP/vositables.py:214 introduced by 9024259. The lint step runs before pytest, so the whole test matrix aborts before a single test executes. requirements-test.txt asked for `ruff>=0.1`, so CI silently tracked whatever ruff had shipped most recently; a newer ruff began flagging this line on code nobody had touched. Pin ruff==0.15.2 so rule changes arrive as a deliberate bump rather than a surprise red build. The vositables.py change is whitespace-only -- `git diff -w` is empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The existing VOSI tests go through the session-scoped `tap_server` fixture, which shares one TAP_WORKDIR across the whole suite. By the time they run, earlier sync requests have already created <workdir>/TAP, so a full-suite run only ever exercised the established-server path -- the fresh-deployment case, where the workspace-resolution bug actually returned a 500, was covered only by accident when the file happened to be run in isolation. Add test_vosi_works_without_existing_workspace, which drives the CGI directly against a pristine per-test workdir and asserts both that the response opens with a CRLF-terminated 200 and that the endpoints create no workspace at all. Verified it pins the second defect specifically: reverting only the dispatch move (keeping the header fix) fails these two tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review preference from @jpl-jengelke on #26, mirrored here to keep the two branches in sync. Make the CRLF requirement explicit at each print rather than relying on a trailing \r plus print's implicit \n. Byte output is unchanged -- both forms emit `...\r\n`, and the wire-level tests in tests/test_vosi_http_response.py still pass unmodified -- but the intent is legible without having to remember what print appends, which matters on a file where a bare LF is what broke the endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The NEA deployment already answers /capabilities and /availability with application/xml, and that is the type we want for these documents. Match it here so the two do not diverge, and update the wire-level assertion to match. Scope is deliberately the two VOSI emitters this PR already touches; the other Content-type sites (including __printVosiTables__ and the error paths) are left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three things stood between the test fixture and the async endpoint, none of them related to each other: - The composed job URLs were malformed. configparam.py appends HTTP_PORT to HTTP_URL for any port other than 80/443, and the fixture had the port in both, so the status URL came back as host:port:0. No sync test reads httpurl, which is why this never surfaced. Split the template into TEST_HTTP_HOST and TEST_HTTP_PORT. - A CGI that dies has only stderr to say so, and the fixture handler dropped it, which makes a 500 from the script indistinguishable from a 500 the script meant to send. Surface it on the runner's stderr. - tap.py asks BeautifulSoup for the 'lxml' tree builder by name when it reads a status document (TAP/tap.py:904, 2162), so bs4 alone is not enough for anything that touches an existing job. Add lxml to the test requirements. (setup.py's install_requires is missing it too, along with bs4, xmltodict, astropy and sqlparse; that is a packaging fix for its own PR.) Also strips one trailing space in TAP/vositables.py that came in with the v3.0.1 merge and is currently failing `ruff check .` on develop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5cRrMXoPJ6zSiGfKYcqjC
An async submit has to finish an HTTP response now and keep executing the query afterwards. Under CGI those pull against each other: the server completes the response when the script closes stdout, and mod_cgi terminates the script once the request is cleaned up. So the process that runs the query can be neither the one holding stdout nor a member of the request's process group. __printAsyncResponse__ resolved that by sending the 303, sleeping two seconds, and then SIGKILLing os.getppid(). Under CGI that parent is the web server child serving the request. Killing it does end the response, since the body carried no Content-Length, and it does orphan the CGI so the job survives. But the client's connection dies mid-message, and behind a reverse proxy the proxy is the one holding that connection: it marks the backend in error and answers 5xx for the next request or two routed over it. Bare mod_cgi respawned the killed child silently, so the damage only became visible once a proxy was put in front. Fork instead. The parent records the child as the job's runId, writes the status document, sends the 303 and exits, which ends the response the way any other CGI does. The child calls setsid() to leave the request's process group, points its standard streams at /dev/null, and runs the query. Three details are load-bearing: - The child waits on a pipe until the parent has published the status document. Without that, a fast query can write COMPLETED before the parent writes EXECUTING, and the job looks like it regressed. - The child redirects stdio before it waits, so it never holds a dup of the server pipe (which would delay the response) and can never write into a response that has already been sent. - uws:runId now carries the child's pid, because ABORT kills whatever runId names (TAP/tap.py:1125-1136). A stale pid there would either do nothing or, worse, signal an unrelated process. The response is also self-delimiting now: an nph- script gets no Content-Length from the server, and a proxy has no other way to know where the message ends. If fork() fails, the request is answered from this process and the query runs here, which is the old behavior minus the kill: correct response, job tied to the request's lifetime. tests/test_async_flow.py covers the full three round trips (submit, PHASE=RUN, poll to COMPLETED plus a non-empty result on disk), asserts the 303 is framed, and guards the regression at the source. That last one is a source assertion on purpose: the behavioral version is "the process running this suite is still alive", which a suite that has been SIGKILLed cannot make. Before this change, running these tests printed "Killed" and nothing else. tests/test_pyneid_compat.py recorded that pyNEID "hung at step 2 with no observable error in the TAP debug log". Step 2 is PHASE=RUN. Its docstring is corrected to say what the hang actually was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5cRrMXoPJ6zSiGfKYcqjC
Both from Copilot's review, both real. The async block ends in one shared response path, and every phase reaches it. ABORT and unrecognized phases decide their terminal status, hit that path, and then fall through into the query, which overwrites ABORTED (or ERROR) with COMPLETED. So a client that aborted a PENDING job was told ABORTED and got its results anyway. That predates this branch: the old code wrote the status, sent the 303, killed the server and then ran the query in the orphan. The fork made it easier to see, not worse. The dead per-branch response calls still visible in the RUN and ABORT branches show the original intent, hoisting them into a shared tail is what dropped the exit. Terminal phases now write their status, answer the client, and sys.exit(), which is what __writeAsyncError__ and __printError__ already do everywhere else in this file. Only RUN detaches. Second: the parent released the child after sending the response, so a client or proxy that hung up mid-write took the exception path, skipped the release, and the child saw EOF and exited, leaving a published job stuck in EXECUTING forever. The release moves into a finally and is gated on the status write having succeeded: a hung-up client does not make the job go away, but if there is no job document there is nothing for the child to update. Tests cover both terminal phases, including that nothing runs afterwards (phase holds and no result file appears). Both fail against the previous commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5cRrMXoPJ6zSiGfKYcqjC
configparam always passes tap_schema_file='TAP_SCHEMA.db', SQLite's ATTACH name. Since 4b35392 TableValidator and dataDictionary preferred it for every DBMS, so Oracle got 'FROM TAP_SCHEMA.db.tables' (ORA-03048) on every query. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
A 1.x TAP.conf ([webserver]/[<dbms>]/ADQL_*) is translated to the 3.x layout in memory and turns on every NEA compat behavior; a [WEB] config may list them in COMPAT. Unknown names fail at startup. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…t types) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Under nea-uws the 303 that starts a job carries no Content-Type, Content-Length or Connection line, as bbc1108 prints it; the response ends when the CGI exits. Default mode bytes are unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…T only in [WEB] - nea-uws accepts /async/<id>/executionDuration as bbc1108 did (compat.uws_url_key) - status.xml is read with either duration spelling, so jobs written under one mode survive a switch to the other (compat.duration_value, compat.uws_key) - COMPAT in any section but [WEB] (including 1.x [webserver]) is a config error Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NEA compatibility mode, TAP_SCHEMA.db Oracle fix, deprecation path (3.1.0)
…ling-server Detach async jobs by forking instead of SIGKILLing the web server
Default mode sends the status line and Content-type this branch adds; nea-vosi-headers adds NEA's Connection: close and trailing CRLF. The fresh-workdir test fills in the TEST_HTTP_HOST/PORT template fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fix malformed HTTP responses from /capabilities and /availability
sqlparse, astropy, beautifulsoup4, lxml and xmltodict are needed at run time but were listed only in requirements-test.txt, so CI (which installs with --no-deps) passed while a real pip install produced a service whose async jobs all failed: BeautifulSoup asks for the lxml parser by name. Database drivers stay undeclared; each deployment installs its own. tests/test_install_requires.py fails when TAP imports a package that setup.py does not declare. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Declare runtime dependencies in setup.py (lxml, bs4, xmltodict, sqlparse, astropy)
translate_legacy copied only COOKIENAME, RACOL and DECCOL from [webserver] to the connection section, so a 1.x config's PROPFILTER, ACCESS_TBL, USERS_TBL, FILEID and ACCESSID were dropped and the proprietary filter was silently off. Every [webserver] key that is not a [WEB] or ADQL_* key now goes to the connection section. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fix: 1.x config translation dropped the proprietary-filter keys
taplint (E-UWS-DENO, -HTDE, -JDED, -JDDE) reports four async problems: - Deleting a job did nothing: POST ACTION=DELETE returned the job, and HTTP DELETE was a 500 because FieldStorage raises 'not indexable' on a request without form fields. Both now stop a running job, remove its workspace and answer 303 to the job list; the job is then 404. - An unknown job id was a 500; it is 404. Job ids must match the workspace name pattern (tap_...), so no id reaches outside <workdir>/TAP. - The job document wrote <uws:executionduration>; UWS names it executionDuration. Reading accepts both, for jobs written earlier. - An unset value read back as the text 'None' (/destruction before a job runs) while the job document had it empty; both are now empty. GET on the job list returned a new job; it now returns an empty <uws:jobs>, since clients follow the deletion redirect there. Job URLs join HTTP_URL and CGI_PGM with one slash (NEA keeps its form). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
UWS conformance: job deletion, 404 for unknown jobs, executionDuration, consistent job values
cx_Oracle 8 reports DATE columns as DB_TYPE_DATE, which matched only the 'DATE' check, so the IPAC writer gave them a fixed 14-character width. Oracle DATE values include a time of day (YYYY-MM-DD HH:MM:SS), so every IPAC-format query returning a DATE column failed with "overflows column width [14 char]". cx_Oracle 7 reported these as DATETIME (30 characters); restore that. CSV and VOTable output were unaffected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 3.0.2 hotfix (fcab318) was made on main from v3.0.1 and never reached develop, so 3.1 builds lacked it. Bring it over: - group the whole user WHERE predicate before the proprietary constraint is applied, and reject query shapes the filter cannot rewrite safely (aliases, joins, subqueries, comments), checking every referenced table - bind cookie user IDs for Oracle and PostgreSQL - stop logging passwords, cookies and request values; only the server environment (TAP_DEBUG=1) can turn on request debugging - add tests/test_propfilter_security.py Conflicts kept develop's form handling (DELETE requests) and logging style. Also drops a password debug line that compat_vositables.py had gained. The tests now give Tap a FieldStorage-like form, since develop checks form.list. Release metadata, HOTFIX-3.0.2.md and the 3.0.1-only conftest stay on main. ruff passes; 238 passed, 2 skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ubuntu-latest now resolves to Ubuntu 26.04, where actions/setup-python has no Python 3.9 build, so test (py3.9) failed during setup on every PR. Pin the test job to 24.04 to keep 3.9 coverage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI: run the test matrix on ubuntu-24.04
Size Oracle DATE columns like DATETIME in IPAC-table output
Port the 3.0.2 proprietary-access fixes to develop
Brings the v3.0.2 hotfix history into the 3.1 line. develop already has the hotfix's code (ported in #35), so conflicts keep develop's version. HOTFIX-3.0.2.md and its MANIFEST.in entry are 3.0.2-only and are dropped. Version strings in Sphinx/conf.py and the tap.py debug banner now read 3.1.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Releases the 3.1 line from
develop. The latest release is v3.0.2;setup.pyondevelophas said 3.1.0 for a while, but it was never released.What's in 3.1.0 since 3.0.2
compat.py,compat_vositables.pyandMIGRATING.md, which translate 1.x/3.0 configs, including every 1.x[webserver]key (NEA compatibility mode, TAP_SCHEMA.db Oracle fix, deprecation path (3.1.0) #30, Fix: 1.x config translation dropped the proprietary-filter keys #32).executionDuration, consistent values (UWS conformance: job deletion, 404 for unknown jobs, executionDuration, consistent job values #33).setup.py(Declare runtime dependencies in setup.py (lxml, bs4, xmltodict, sqlparse, astropy) #31).DATEcolumns in IPAC-table output: sized like DATETIME again (Size Oracle DATE columns like DATETIME in IPAC-table output #34).This PR
mainis merged into the release branch so v3.0.2's history comes along.developalready has the hotfix's code (#35), so the conflicts keepdevelop's version. Apart from that, it only:HOTFIX-3.0.2.mdand itsMANIFEST.inentry, which are 3.0.2-only;Sphinx/conf.pyand thetap.pydebug banner to 3.1.0.After this merges: tag
v3.1.0onmain, publish the GitHub release, and mergemainback intodevelop.Testing
pytest tests/gives 238 passed, 2 skipped.🤖 Generated with Claude Code