Loris icon indicating copy to clipboard operation
Loris copied to clipboard

[API] Add dicom upload

Open maximemulder opened this issue 1 year ago • 12 comments

Brief summary of changes

This is a port of #6161, which contains a description of the changes. Since the old PR has diverged quite a bit from the main branch, and that its author no longer works here, I found it easier to copy the changes on the current main branch and open a new PR rather than to rebase the PR. While porting, I only rewrote the old PR code whenever static analysis or practical stests showed that it was no longer in sync with the main branch (or when my IDE removed trailing spaces).

Testing instructions (if applicable)

  • Test the four new API routes, described in section 5 of the documentation of the API v0.0.4.

Notes for existing projects

This PR requires to run the following SQL patch SQL/New_patches/2024-03_19-MRIUploadServerProcessRel.sql.

maximemulder avatar Mar 20 '24 20:03 maximemulder

I must say I am not sure what I am supposed to do regarding the failed tests on files that I did not modify in the PR. Any direction @driusan (or someone else) 🤔.

maximemulder avatar Mar 20 '24 20:03 maximemulder

Line php/libraries/FilesDownloadHandler.php
71 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, mixed given.

Line php/libraries/SiteIDGenerator.php
81 Negated boolean expression is always false.
💡 Because the type is coming from a PHPDoc, you can turn off this
check by setting treatPhpDocTypesAsCertain: false in your
./test/phpstan-loris.neon.
297 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, mixed given.
377 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, array<int,
int|string>|string|null given.
387 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, array<int, int|string>|string
given.
400 Parameter #1 $value of function strval expects
bool|float|int|resource|string|null, array<int, int|string>|string
given.

kongtiaowang avatar Apr 24 '24 18:04 kongtiaowang

Hum, yeah, I already saw that.

I was just suprised to see that the errors are marked as originating from FilesDownloadHandler.php and SiteIDGenerator.php, which are not modified by this PR. I guess I must call these files from elsewhere and that the location reporting is just imprecise.

I'll tackle this tomorrow probably.

maximemulder avatar Apr 24 '24 19:04 maximemulder

I've looked at the static errors in more details, it appears to be mainly due to an upgrade of phpstan, which changes the typing of string conversions with strval and (string) from mixed -> string to (bool|float|int|resource|string|null) -> string. I still don't know why this PR triggers the error and not other ones (it did trigger on my main branch though), but at least the errors are clear.

However, I find the code that triggers the errors to be quite cryptic, with some lines that seem obviously wrong (at least according to typing). If someone who knows this code better than me could look into it, it would be best. In the meantime, I'm adding this to the next meeting.

maximemulder avatar May 22 '24 17:05 maximemulder

I still don't know why this PR triggers the error and not other ones (it did trigger on my main branch though), but at least the errors are clear.

Probably because composer update was run, which triggered an update of the static checker.

laemtl avatar May 28 '24 15:05 laemtl

Ok it seems the linting errors are ~~fixed~~, silenced. @laemtl had noticed that the errors were due to a composer update on my end. The errors should still appear in any PR that updates composer and should be fixed eventually. However, this can have its own issue and be another PR.

maximemulder avatar May 28 '24 18:05 maximemulder

@ridz1208 Ok I finally finished updating and re-testing this PR. Generally it seems to work. However, if the pipeline fails during the latter stages, the DICOM archive previously inserted does not have its session initialized. This should be fixed with the new DICOM archive PR, but I don't expect it to be merged before at least a few weeks.

maximemulder avatar Aug 29 '24 16:08 maximemulder

Please remove all the unrelated changes (whitespace, variable name changes, etc). If necessary you can send those in another PR. It is not feasible to review this PR with so many unrelated changed mixed together.

driusan avatar Sep 04 '24 16:09 driusan

I replied to all the comments and applied the suggested changes.

I reverted the breaking changes to the API including floats / strings and naming. Back when I wrote them, I thought it was part of the original PR and not something that was already in LORIS.

I made the terminology more consistent by using $VisitLabel everywhere in the DICOM part (section 5) of the API documentation.

I also disabled my trailing spaces auto-removal and removed the associated changes.

Ready for review @cmadjar @driusan

maximemulder avatar Sep 17 '24 20:09 maximemulder