add request option to retrieve instances

[LachlanNewman]

Provides the same functionality to pass an instance of a request like 'storeInstances` does.

[pieper]

This seems very reasonable to me, but I'd like it if someone else could have a look and let us know if there are an concerns.

[pieper]

@LachlanNewman one issue is that the commit message should be in the semantic release format so this triggers a feature release.

https://github.com/dcmjs-org/dicomweb-client?tab=readme-ov-file#for-maintainers

[LachlanNewman]

@pieper i have amended the commit message. is there anything that needs to be done ?

[pieper]

@LachlanNewman Thanks for doing that. I'm not 100% sure, but I think the message should start with "feat(request): " to be sure the release will be triggered.

Also I do want to hear from either @sedghi or @wayfarer3130 since they would be more aware than I of any possible compatibility issues.

[wayfarer3130]

Adding a request object constrains the implementation to use http requests, and also has this magic setup where things like data to be uploaded is configured in some http request. I'd rather see the core library be enhanced with specific pieces of functionality to allow specifying data to upload as a callback method, specifying a modifier for the method/attributes/url. That would give more functionality that was much more targetted. If you really want to have the request specified, it should be done as a createXmlHttpRequest method, but I'd rather have individual methods which take information about the request being generated and modify the basic/underlying request. The parts to modify:

1. HTTP headers
2. URL path
3. Creation of the request (to allow using a completely different type of request, eg a file URL or something like that)
4. Upload data to send, as an iterator on an upload set, returning fixed data or an input stream, or perhaps a writeable output stream consuming the input stream, and having breaks for multipart tags.
5. Streaming response handler (to break the stream up into multiple pieces)

These should be in an options argument, not added as single argument changes that would make future releases incompatible when other types of attributes got added.

For example,
_httpGetMultipartApplicationDicom(
url,
mediaTypes,
params,
progressCallback,
withCredentials,
request
) {

could be:

_httpGetMultipartApplicationDicom(
url,
mediaTypes,
params,
options?: {
progressCallback,
withCredentials,
createCustomStream,
urlRewriteHandler,
attributeGenerator,
sendDataGenerator,
paramRewriteHandler,
multipartCallback,
}

The set of options should be fixed, declaring it in a type object and should apply to the various methods consistently.
In your particular case, createCustomStreamHandler might be what you want if you are not talking http, but most likely you just want to send a generator or handler. That allows for much more targeted changes to be applied. Generators create additional data, while handlers modify the data, and callbacks get called with various data, but don't modify it.
I'm not saying you need to implement all of this - but splitting it up makes how each part is handled implementable by itself without necessarily rewriting other parts of the code, and tells the api exactly what types of changes you are making.

[LachlanNewman]

@wayfarer3130 I'm a little confused about what you're asking sorry. The point of this PR was to be able to inject a request object that is compatible with the XMLHttpRequest specification. Are you suggesting passing a call back that allows for creating the requesting object ?

I am happy to refactor the functionality to allow for passing a callback such as createRequest which would be called to generate the HTTP request but it looks like the method _httpRequest is tightly coupled to the XMLHttpRequest API defined here.

[hao-dong-annalise]

Yeah. This PR is to align the API retrieveInstance to be similar to the storeInstances to support an option to pas in the request object so it can be used in nodejs env. Broader refactor to the underline "internal" methods can be done later in other PRs, I would suggest.

[andrearampin]

hey @LachlanNewman thanks a lot for the PR. I was really looking to get this fixed as well. @wayfarer3130 and @pieper would it be possible to get this approved?

[pieper]

Thanks for the review @wayfarer3130 - Looking at the change, does having a request option on the retrieve method preclude any of the things you are proposing? Right now it seems we don't have options for _httpGetMultipartApplicationDicom but this could be added later if the need comes up. I think I see the point of the PR adding an extra element of consistency and uniformity in the api.

[wayfarer3130]

Thanks for the review @wayfarer3130 - Looking at the change, does having a request option on the retrieve method preclude any of the things you are proposing? Right now it seems we don't have options for _httpGetMultipartApplicationDicom but this could be added later if the need comes up. I think I see the point of the PR adding an extra element of consistency and uniformity in the api.

It doesn't preclude it, but it makes the API more messy because options get passed into the functions in various ways.
I'd really prefer to go with a consistent options parameter, and just pass in the request inside that. Change the one existing call that has a direct request parameter to use a options.request and then we can go with this, under the proviso that at some point the API probably will get a new major release that removes the request parameter, or that breaks it in some other way. Adding better/correct ways to define it WILL break this method.

[pieper]

Okay, so we want _httpGetMultipartApplicationDicom and _httpGet to take the request in the form of options.request?

Does that work for @LachlanNewman @hao-dong-annalise @andrearampin

[hao-dong-annalise]

Okay, so we want _httpGetMultipartApplicationDicom and _httpGet to take the request in the form of options.request?

Does that work for @LachlanNewman @hao-dong-annalise @andrearampin

We also should refactor other retrieve APIs like retrieveStudy, retrieveStudyMetadata, retrieveSeries, retrieveSeriesMetadata etc. to support the options.request option.

[wayfarer3130]

Okay, so we want _httpGetMultipartApplicationDicom and _httpGet to take the request in the form of options.request?
Does that work for @LachlanNewman @hao-dong-annalise @andrearampin

We also should refactor other retrieve APIs like retrieveStudy, retrieveStudyMetadata, retrieveSeries, retrieveSeriesMetadata etc. to support the options.request option.

Yes, please refactor anything else that takes the request as an option.

[LachlanNewman]

@wayfarer3130 i have add the required changes let me know if everything looks ok.

[LachlanNewman]

@wayfarer3130 do the tests need to pass in order to merge ? this is the issue is think https://github.com/orgs/community/discussions/116610

[wayfarer3130]

@wayfarer3130 do the tests need to pass in order to merge ? this is the issue is think https://github.com/orgs/community/discussions/116610

That is a question for @pieper as I'm not quite as involved in this project so i don't know the test status right now.

[pieper]

Yes, we want the tests to pass, and it looks like the docker compose change has fixed them 👍

[github-actions]

🎉 This PR is included in version 0.11.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀