Repository navigation
Conversation
Adds a study-scoped, collection-level REST API alongside the existing
/api/* routes, which are unchanged and still serve the frontend.
Seven collections (resources, devices, designs, plasmids, strains,
sample-designs, assays) each get GET/POST/PUT/DELETE plus GET /template;
assays additionally get PATCH /{id}/attachments. The handlers are generic
and bound to each collection from a registry, so adding a collection is
one entry in registry.py. models, archives and analyses are registered
but unimplemented and answer 501.
Requests carry the SynBioHub token as Authorization: Bearer and the study
collection as X-Study-Collection. Uploads accept SBOL (.xml, .rdf, .sbol)
and Excel (.xlsx, .xlsm); Excel is converted to SBOL via excel2sbol before
submission.
SynBioHub semantics, verified against a live instance:
- DELETE empties a collection using removeMembership, which unlinks
members without destroying the objects. A later GET is 200 [].
- /submit rejects overwrite_merge=1 when rootCollections is set, so PUT
cannot be a single atomic submit: it validates and converts first,
then clears, submits, and restores membership if the submit fails.
- 404 is reserved for a missing study collection; a missing
sub-collection is an empty result, not an error.
Two integration points with the current backend:
- upload_to_sbh now takes sbh_prefix, the URI namespace an object must
already live under to be treated as existing on SynBioHub rather than
created by this upload. The legacy routes read it from the request
body; StudyContext derives it from the study URI's scheme and host,
which is a URI in exactly that namespace. This is the web host, not
sbh_url -- the same distinction upload_sbh_attachments draws.
- build_template, extracted from sbh_download_template so the API and
POST /api/downloadTemplate share one implementation, carries over the
short-circuit that returns the resources template byte-for-byte
instead of round-tripping it through openpyxl.
Also in this change:
- Replaces static/swagger.json, which documented seven endpoints that
do not exist, with static/openapi.yaml describing what is actually
served. Swagger UI is mounted at /api/docs and /api/v1/docs.
- Pins excel2sbol to the commit the conversion path was tested against,
and declares pandas and openpyxl, which are imported directly but
previously arrived only transitively. Drops excel2flapjack, which is
never imported and conflicts with excel2sbol on pandas.
- Bounds uploads with MAX_CONTENT_LENGTH (SBS_MAX_UPLOAD_MB, default 64)
and sets a uwsgi harakiri above the observed worst-case conversion.
- Converts workbooks against EXCEL_HOMESPACE rather than the collection's
own homespace, so /api/v1 and route.py:xdc_run put an object on the same
URI before submission. EXCEL_HOMESPACE was previously referenced only by
a docstring claiming behaviour the code did not have.
- Adds 208 tests and a CI job to run them on PRs to master. Tests fake
SynBioHub by default; the 21 live tests skip unless backend/.env.test
is present. Among them the first tests that actually run excel2sbol --
conversion had no execution coverage at all -- plus live coverage for
attachments, and assertions that pin the resources template short-circuit
and the contents of a PUT rollback.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://witty-hill-08172a210-489.centralus.5.azurestaticapps.net |
Contributor
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
|
@PranavM06 please install https://github.com/affaan-m/ECC and make a review to the code. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfd9ab7f7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| upload_to_sbh( | ||
| doc, | ||
| ctx.sbh_url, | ||
| ctx.sbh_prefix, |
There was a problem hiding this comment.
When a PUT uploads SBOL whose object identities already use the study's SynBioHub host (for example, objects previously downloaded from that hub), passing ctx.sbh_prefix causes upload_to_sbh to exclude every such object from the generated sub-collection membership. Because replace() has already cleared the collection, its subsequent membership query is empty, so SynBioHub accepts the submission and the API returns 200 while the collection remains empty instead of containing the uploaded objects.
Useful? React with 👍 / 👎.
Comment on lines
+170
to
+172
| clear_collection(ctx, spec) | ||
| try: | ||
| submit(ctx, spec, doc, upload_dir, MERGE_AND_OVERWRITE) |
There was a problem hiding this comment.
When a PUT replaces a collection with multiple members and a later removeMembership request fails after earlier removals succeeded, clear_collection() raises here before entering the surrounding try, so restore_members() is never called. The request returns 502 but leaves the original collection partially emptied, contradicting the rollback protection described for PUT; the clear step needs to be included in the rollback scope.
Useful? React with 👍 / 👎.
Comment on lines
+90
to
+91
| if object_id in (display_id, uri) or uri.rstrip('/').endswith(f'/{object_id}'): | ||
| return uri |
There was a problem hiding this comment.
When the attachment path contains a value equal to a member URI's final segment, this suffix fallback resolves an unrelated member even without a matching displayId. SynBioHub member URIs normally end in a version such as /1, so PATCH /assays/1/attachments selects the first result returned by SPARQL and attaches experimental data to the wrong assay instead of returning 404.
Useful? React with 👍 / 👎.
Comment on lines
+40
to
+41
| return bp | ||
|
|
There was a problem hiding this comment.
When a client uses the full object URI advertised by openapi.yaml, Flask's default string converter cannot capture its slash-containing value, so routing returns 404 before resolve_member() can perform its explicit full-URI comparison. Use a path-capable converter or stop advertising full URIs as accepted identifiers.
Useful? React with 👍 / 👎.
PUT used to snapshot, clear with removeMembership, submit, and restore on failure. A failure mid-clear was never rolled back, members added between the two queries were lost, and a failed restore only reached print(). Now PUT is one /submit (overwrite_merge=3) whose sub-collection lists only the upload. SynBioHub replaces an existing object whose content differs, member list included (SynBioHub 8d99c34: PrepareSubmissionJob.java:543 replaceSBOL -> /replace -> remove.sparql, then submit.js:485 inserts), so nothing is cleared first and there is nothing to roll back. upload_to_sbh gains replace_members to skip the re-list POST still needs. SynBioHub only logs a failed replace and still answers 200, so PUT counts the sub-collection's member links afterwards and returns 502 if old members remain. It never unlinks anything itself. The live PUT test now doubles as the probe (two POSTs accumulate, PUT leaves only its object, the old objects survive); it has not been run.
sbol2's VALIDATE defaults to True, so every Document.write() POSTed the whole document to validator.sbolstandard.org with no timeout. excel2sbol ends conversion with doc.write(), and the legacy upload_to_sbh path wrote the same way. Every caller discards the verdict, so validation only leaked user data and let a validator outage fail or hang uploads. - utils.sbol_validation_disabled(): scoped off-switch that restores the previous value. Used around excel2sbol.converter in convert_to_sbol and around doc.write in upload_to_sbh (covers /api/v1 and legacy routes); api/sbh.submit's inline toggle is now redundant and removed. - conftest: sbol_file no longer sets VALIDATE=False for the whole process (that hid this bug); an autouse fixture restores sbol2's Config after every test, so each test starts from sbol2's defaults. - tests/fixtures/resources_synthetic.xlsx: made-up rows on the public Excel-to-SBOL Resources template (BSD-3; generator script and notice alongside), so real excel2sbol conversion is tested in CI instead of skipping for want of files under ~/SPUR.
excel2sbol (5910698) GETs every URL in a Sequence or (URI) cell (library2.link_validation) and, when the welcome sheet sets a Domain, searches that host for every displayId and sequence. No timeout, and the uploaded workbook picks the host. A 401/403/404 set os.environ COUNTER, which nothing cleared, so every later conversion in that uwsgi worker wrote nothing and failed with 400; other statuses reached sys.exit(1), whose SystemExit got past every except Exception. SBOL_DICTIONARY also outlived the request: reproduced offline, a workbook with no welcome sheet sent its displayIds to the previous uploader's Domain. No response ever reaches the document -- each request only decides whether to warn, skip writing or exit -- and none of the 23 local workbooks (lab, public templates, fixture) makes a request. So the checks are switched off rather than routed through sbh_http, which would still let an uploader's public host be contacted. - utils.excel2sbol_offline(): stubs link_validation to "reachable" (the URL is kept as a reference, as before), drops the welcome sheet (its only use is the Domain lookups), makes HTTPAdapter.send refuse anything else, and clears COUNTER and SBOL_DICTIONARY before and after each run. Process-global like sbol_validation_disabled; fine under processes = 4, no threads. - convert_to_sbol uses it (covers /api/v1 and legacy xdc_run) and turns SystemExit into an exception, so it becomes a 400. Behaviour change: URL cells are no longer checked, and the welcome sheet's Domain/Library Name/Master Collection are ignored. excel2sbol reads "Previous Version (URI)" only when a Domain is set, so Domain workbooks lose it; Domain-less workbooks, every real one, already did.
tyto answers a column with "Tyto Lookup" TRUE by querying Ontobee (sparql.hegroup.org) through SPARQLWrapper, i.e. urllib -- which the HTTPAdapter.send refusal in excel2sbol_offline() never saw -- and sets no timeout, so a silent Ontobee held a uwsgi worker until harakiri. None of the shipped templates turn it on; older SBOL2 parts templates do, for Source/Target Organism in NCBITaxon, and convert today, so Ontobee stays reachable under a cap rather than being stubbed. - excel2sbol_offline() now also works at socket level: getaddrinfo refuses every name but Ontobee's before DNS and keeps only its public addresses; connect/connect_ex refuse every other address. Ontobee sockets get ONTOBEE_TIMEOUT (10 s) per operation, and none starts after ONTOBEE_BUDGET (30 s) into the conversion. - Each (ontology, term) is asked once per conversion: tyto caches nothing and workbooks repeat the organism on every row. - A transport failure becomes OntologyLookupError, which ingest answers with a 504 naming Ontobee. A query Ontobee rejects stays a 400. - tests: _block_network now refuses every non-loopback connect (urllib, requests.Session, raw sockets) and fails the test at teardown on any attempt, so an except Exception in the code under test cannot hide one. Offline, with DNS stubbed, Excel-to-SBOL's ACS parts template asks only for sparql.hegroup.org and ends in a clean 504 in 0.4 s. Ontobee's real latency is unmeasured (no live calls); ONTOBEE_TIMEOUT is the knob.
A requests timeout from upload_to_sbh fell into the catch-all and came back as 502 "SynBioHub rejected the submission", though SynBioHub may still be storing the upload. - Read timeout: 504 saying the outcome is unknown, that resending the same request is safe (the same objects and, with overwrite_merge=3, the same member list), and to GET the collection to check. A PUT that times out skips the after-submit member check: there is nothing to verify yet. - Connect timeout: 504 saying nothing was submitted. - A POST whose member query (before the submit) times out also says "unknown", though nothing was sent: the safe side, and it keeps upload_to_sbh and the legacy routes unchanged. - api/sbh.py imports only the two exception classes, so it still cannot call SynBioHub around sbh_http. - openapi.yaml: a GatewayTimeout response on every POST/PUT that reaches SynBioHub. - tests: FakeSynBioHub raises a registered exception, as requests does. Behind the base image's nginx (~60 s read timeout) clients still get nginx's own 504 before this one; left with the deploy questions.
| with sbol_validation_disabled(): | ||
| raise RuntimeError('write failed') | ||
|
|
||
| assert validating() is True |
| response = getattr(client, method)( | ||
| '/api/v1/devices', | ||
| headers=auth_headers(**{SBH_URL_HEADER: listener.url}), | ||
| data={'file': (open(sbol_file, 'rb'), 'part.xml')}, |
| return client.post( | ||
| '/api/uploadSBOL', | ||
| data={ | ||
| 'SBOL': (open(sbol_file, 'rb'), 'part.xml'), |
| output.seek(0) | ||
| sbh_url = normalise_base_url(params_from_request['sbh_url']) | ||
| except ValueError as e: | ||
| return jsonify({"error": f"Invalid sbh_url: {e}"}), 400 |
| ) | ||
|
|
||
| except BlockedDestinationError as e: | ||
| return jsonify({"error": str(e)}), 400 |
| try: | ||
| sbh_url = normalise_base_url(params_from_request.get('sbh_url')) | ||
| except ValueError as e: | ||
| return jsonify({"error": f"Invalid sbh_url: {e}"}), 400 |
| # Also adds https:// when the scheme is missing, as this route always has. | ||
| sbh_url = normalise_base_url(params_from_request['sbh_url']) | ||
| except ValueError as e: | ||
| return jsonify({"error": f"Invalid sbh_url: {e}"}), 400 |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://witty-hill-08172a210-489.centralus.5.azurestaticapps.net |
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
3 participants
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.
Adds a study-scoped, collection-level REST API alongside the existing /api/* routes, which are unchanged and still serve the frontend.
Seven collections (resources, devices, designs, plasmids, strains, sample-designs, assays) each get GET/POST/PUT/DELETE plus GET /template; assays additionally get PATCH /{id}/attachments. The handlers are generic and bound to each collection from a registry, so adding a collection is one entry in registry.py. models, archives and analyses are registered but unimplemented and answer 501.
Requests carry the SynBioHub token as Authorization: Bearer and the study collection as X-Study-Collection. Uploads accept SBOL (.xml, .rdf, .sbol) and Excel (.xlsx, .xlsm); Excel is converted to SBOL via excel2sbol before submission.
SynBioHub semantics, verified against a live instance:
Two integration points with the current backend:
Also in this change: