Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Escape single quote in DICOM database SQL statement #881

Merged
merged 1 commit into from Sep 2, 2019

Conversation

@cpinter
Copy link
Member

commented Aug 29, 2019

@lassoan

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Looks good, but why don't we do it with all the other fields? For example, physician's name can contain single-quote, too.

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

It occurred to me but didn't seem likely even for the physician name. If you give me a list of fields you want me to escape the same way I'll add those.

@lassoan

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

It is very common in person names (O'Leary...) and can easily occur in any description fields. Are there any other restricted characters? Does \ have to be encoded as \?

@pieper

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

I don't think this is the source of the issue - the data was imported correctly in the first place, but the error happened when it converted to the new schema. Can you point to the commits involved in the database update?

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Sorry I had only a little time in the evening and didn't pay enough attention. I'll track this down now.

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Actually it would help a lot if I had that data and could debug into it. Without it it's just guesswork.

@cpinter cpinter force-pushed the cpinter:fix-dicom-database-sql-statement branch from db30c43 to ec9b9f1 Aug 29, 2019

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Anyway, I did some guesswork and pushed a new change :) It escapes the single quote character in every field that is being processed during update.

@pieper

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Thanks for digging into this @cpinter

I still don't understand why this became an issue for converting when the data imported correctly before. Do you have any ideas on that? Without that it makes it hard to know if the fix is correct.

I can't share the exact dataset (PHI) and I haven't had time to check, but I can do more debugging/testing at some point as needed. For now I'd be happy just understanding what changed - can you point to the commits that handle the database update?

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

I also don't understand how the initial insertion can be fine without having to sanitize, but then fails later.

This is the original commit
9c2af28
And a few follow-up commits - basically whatever I committed to CTK lately. This is quite a big change, and I'm not sure how reading it would help. If you have specific questions I'm happy to answer though.

@pieper

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Yes, I was looking through and it seems that this squashed commit has everything lumped together and I don't see what would have led to this issue.

Specifically though if you look at the error message it's in an UPDATE to the Studies table - do you know where that would happening?

 Bad SQL: UPDATE Studies SET DisplayedNumberOfSeries='3', InstitutionName='BRIGHAM & WOMEN'S HOSPITAL', PatientsUID=4, ReferringPhysician='x^x^^^', StudyDate='2019xxxx', StudyDescription='MRI BRAIN FUNCTIONAL', StudyInstanceUID='1.2.xx' WHERE StudyInstanceUID='1.2.x'
@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Maybe this PR is not enough after all. We need to make sure the displayed field generator does not override the original value with some problematic value. It's here:
https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMDisplayedFieldGenerator.cpp#L116

@pieper

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Oh well yes, this is where the problem is: https://github.com/commontk/CTK/blob/master/Libs/DICOM/Core/ctkDICOMDatabase.cpp#L1066

This uses QString formatting to build the SQL query instead of the usual prepare and bind that is used elsewhere. It should be changed to match the rest of the QSqlQuery usage and the problem should go away.

@pieper

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Also line 1144 and anywhere else where sql statements are created.

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

OK. I'll change the queries to use prepare everywhere in this class.

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Line 1144 seems to be just a closing brace...

@pieper

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Line 1144 seems to be just a closing brace...

Sorry, meant 1104, but you have the idea. 😄

I don't have the exact commit handy but I ran into this exact issue with the code when it was first written back seven or eight years ago when first testing it with data from Brigham and Women's Hospital and that's why we changed to using the prepare and bind calls.

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

One question, maybe you know this by heart. The prepare/bind way just replaces strings, or is there something "more intelligent" going on there? Because the displayStudiesFieldUpdateList and its similar counterparts are longer strings involving multiple fields, and if it's just string replacement then it's fine, but if it somehow forces to bind values for fields, then it probably won't work like this.

@cpinter cpinter force-pushed the cpinter:fix-dicom-database-sql-statement branch from ec9b9f1 to 729b1f7 Aug 29, 2019

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

I updated the PR. It is not working yet (crashes), but it will be something like this. I have to go now though, sop I'll fix this tomorrow.

@cpinter cpinter force-pushed the cpinter:fix-dicom-database-sql-statement branch from 729b1f7 to f0ea905 Aug 29, 2019

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

This should do it. @pieper can you please test with your dataset?

@pieper

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

Hmm, sorry, no, still not working. But it's closer. Now the error is:

SQL failed
 Bad SQL: UPDATE Studies SET DisplayedNumberOfSeries = ? , InstitutionName = ? , PatientsUID = ? , ReferringPhysician = ? , StudyDate = ? , StudyDescription = ? , StudyInstanceUID = ? StudyInstanceUID UID = ? ;
Error text:  Parameter count mismatch
QString("UPDATE Studies SET %1 WHERE StudyInstanceUID='%2';").arg(displayStudiesFieldUpdateList).arg(currentStudy["StudyInstanceUID"]);
this->loggedExec(updateDisplayStudyStatement, updateDisplayStudyStatementString);
QSqlQuery updateDisplayStudyStatement(this->Database);
updateDisplayStudyStatement.prepare( QString("UPDATE Studies SET %1 StudyInstanceUID UID = ? ;").arg(displayStudiesFieldUpdateString) );

This comment has been minimized.

Copy link
@pieper

pieper Sep 1, 2019

Member

Doesn't this sql statement need a WHERE clause?

@pieper

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

I took a look but didn't see anything obvious here about the parameter counts, but maybe it's related to the comment I added in the code about a missing WHERE clause? We can chat again about it if you want to walk through the logic together.

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2019

Interesting. I tried a simple import then update and it worked for me. Based on the error message, a WHERE seems to be missing. Not sure what's going on. Is it OK if I check it out on Tuesday?

@pieper

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

Oh yes, Tuesday sounds great!

@cpinter cpinter force-pushed the cpinter:fix-dicom-database-sql-statement branch from f0ea905 to ebd2b26 Sep 1, 2019

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2019

I changed the statement. I think I accidentally pasted the StudyInstanceUID on the WHERE instead the UID... Not sure how it worked when I tried it. I hope it's going to be good now.

@pieper

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Tested and it looks good to me - thanks @cpinter! 👍

@pieper pieper merged commit be071a0 into commontk:master Sep 2, 2019

2 checks passed

ci/circleci: build-qt4 Your tests passed on CircleCI!
Details
ci/circleci: build-qt5 Your tests passed on CircleCI!
Details
slicerbot pushed a commit to Slicer/Slicer that referenced this pull request Sep 2, 2019
BUG: fix single quote escape in CTK SQL DICOM
Addresses an issue when there's an apostrophe
in a field name (e.g. Briham and Women's)

See: commontk/CTK#881

$ git shortlog 6a8d584d3eb0ed750f6a850ab4f9c1089820f701..9e89c22d8599763bc20960d1b22175e465fac65c --no-merges
Csaba Pinter (1):
      BUG: Escape single quote in DICOM database SQL statement

Steve Pieper (1):
      STYLE: remove ^M characters

From: Steve Pieper <pieper@isomics.com>

git-svn-id: http://svn.slicer.org/Slicer4/trunk@28474 3bd1e089-480b-0410-8dfb-8563597acbee
@pieper

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Okay, I updated Slicer to use this, so we should be good to go. I'll try to check in the nightly build.

@cpinter

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

Thank you very much! Sorry for the problems!

@jcfr

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.