fix(core): accept contexts in executeUpload, clarify provider targeting docs#373
fix(core): accept contexts in executeUpload, clarify provider targeting docs#373
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses confusing provider targeting in the upload API by allowing callers to pass pre-resolved Synapse storage contexts through executeUpload, and by updating option docs to clarify how provider IDs vs data set IDs vs contexts should be used.
Changes:
- Add
contexts?: StorageContext[]support toexecuteUploadanduploadToSynapseby passing it through to the Synapse SDK upload call. - Add runtime validation in
executeUploadto prevent combining multiple targeting mechanisms (contexts,providerIds,dataSetIds). - Update option documentation across upload-related interfaces to clarify recommended targeting patterns.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/core/upload/synapse.ts | Introduces contexts to the lower-level Synapse upload helper and forwards it into SDK upload options. |
| src/core/upload/index.ts | Adds contexts to executeUpload, validates mutually-exclusive targeting options, and forwards contexts into upload options. |
| src/common/upload-flow.ts | Updates docs/comments for provider/data-set targeting to reduce DX confusion. |
Comments suppressed due to low confidence (1)
src/core/upload/synapse.ts:286
SynapseUploadOptions.contextsis documented as mutually exclusive withproviderIds,dataSetIds, andcopies, butuploadToSynapsecurrently forwards all of these fields independently. SinceuploadToSynapseis a public helper (used outsideexecuteUpload), consider adding a mutual-exclusion guard here as well (or prioritizingcontextsand ignoring the others) to keep runtime behavior aligned with the docs.
// Pass through context selection options
if (options.contexts != null) {
uploadOptions.contexts = options.contexts
}
if (options.copies != null) {
uploadOptions.copies = options.copies
}
if (options.providerIds != null) {
uploadOptions.providerIds = options.providerIds
}
if (options.dataSetIds != null) {
uploadOptions.dataSetIds = options.dataSetIds
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I've also added |
|
Probably should have been a |
Closes: #372
Also discussing in here: FilOzone/synapse-sdk#690
I think the root problem is the migration pain and change of concepts that the SDK is pushing. Dealbot probably doesn't need to be touching contexts directly but we previously encouraged that as the path.
So the "fix" here is twofold: just accept a
contextsargument if you want to provide it, and improve the docs to be much more explicit about what's going on.