I installed the latest nightly, and experienced this situation where DICOM Browser would request me to update the database, but trying to perform the update operation does not appear to have any effect.
Fortunately for me, I did not have a need to keep the database, and just created a new one, which showed up empty. After adding one study to that new database, my prior content re-appeared.
I do not know if this is expected behavior, and I could not find similar posts.
Definitely it can be a regression from my last PR, thanks for reporting @fedorov !!
Unfortunately the bug was not flagged either by the automated and manual tests. I would be curious to see if the database update works by using the new browser in your case. I clearly remember testing manually the feature, but I’m not sure that I tested it on both browsers.
Anyway, in any case, I’ll fix it asap, but I’m currently in vacation and I’ll be back on Monday (and I’ll make it a priority).
Im wondering if we should keep offering this feature. It works by reimporting the DICOM files, which was OK when the database was just a cache of DICOM tags. However, the database now contains information that cannot be derived from the DICOM files (where was it retrieved from). So, we would need to improve the update process to preserve all these information; or not do it anymore (but automatically create a new empty database whenever the schema changes).
I don’t like the idea of users losing their databases.
Schema changes should be very rare and they shouldn’t cause problems for users who don’t use new features. Maybe these extra fields should be stored separately if they are still subject to change.
The new field for the allowed/denied connections should not be an issue. For databases with the old schema when updated those fields would be simply empty and then the visual browser would use the server settings as default.
On the long term not sure what is the way to go. I generally agree with you, but at the same time losing the database at schema change would not be ideal as Steve pointed out
Thanks for working on this @Davide_Punzo and @lassoan 
On @fedorov 's original report, is there another bug that prevents him from updating the schema? Not clear to me why this would be in a loop when the schema update feature used to work.
Not sure, it should work (at least on the visual DICOM browser I know its working). Andrey also wrote that after adding a new study, the previous content re appeared. So probably the update was done successfully, but then something went wrong (maybe some update signals are broken).
On Monday I will investigate the issue on the default browser and I will let you know!
Ah yes this would be nice to implement
@fedorov @pieper @lassoan I tried to reproduce this issue, but I could not.
Specifically I have tried to update with latest Slicer a database created with Slicer-5.6.2-linux-amd64, i.e. DICOM database from version 0.7.0 to 0.8.1
Here a video:
@fedorov which OS are you using? @pieper could you please try to reproduce this on MacOS? and @lassoan could you try Windows as well, please?
we have also an automated test for this: CTK/Libs/DICOM/Core/Testing/Cpp/ctkDICOMDatabaseTest3.cpp at master · commontk/CTK · GitHub
but the CTK CI is run on linux.
I did an update of a medium sized (~35 patients) database with the versions you mention yesterday on Windows (Slicer 5.7 from March 28), and it worked. The displayed name column content did not show during the update, but did do at the end.
I remember adding a ticket or commenting on one recently related to this (after doing some debugging, which showed a problem related with the displayed field generators - btw I implemented those back in the day), but can’t find it in CTK or Slicer. In any case, just letting know that the update worked for me too.
I did an update of a medium sized (~35 patients) database with the versions you mention yesterday on Windows (Slicer 5.7 from March 28), and it worked. The displayed name column content did not show during the update, but did do at the end.
ok perfect thanks for testing and let me know 
I remember adding a ticket or commenting on one recently related to this (after doing some debugging, which showed a problem related with the displayed field generators - btw I implemented those back in the day), but can’t find it in CTK or Slicer. In any case, just letting know that the update worked for me too.
ok if you find again the issue, please ping me, I will try to have a look!
@Davide_Punzo it looks like the Count column is empty after the update. Can you test if this is a side-effect of updating the database or is it something that happens if you start with a fresh database (i.e. is the count populated in a new database).
I am on macOS Sonoma 14.4.
@Davide_Punzo it looks like the
Countcolumn is empty after the update. Can you test if this is a side-effect of updating the database or is it something that happens if you start with a fresh database (i.e. is the count populated in a new database).
it is a general bug. I have investigated it and the issue is that:
#define DCM_SeriesInstanceUID DcmTagKey(0x0020, 0x000e)"0020,000E"because the return of cachedTagsForInstance[dicomTagToString(DCM_SeriesInstanceUID)] is an empty string (because dicomTagToString(DCM_SeriesInstanceUID) return “0020,000e” instead of “0020,000E”) and this creates issues when updating the tables.
I am not sure if this is a new regression, but I will check out differences in the CTK commits after Slicer version 5.6.2 and see if I can find where the regression happened.
@pieper @cpinter not sure what is the best way to fix the issue. Maybe we should fix the dicoms at loading/fetching time to have lowercase in the tags, instead of putting a lot of if exceptions everytime we use dicomTagToString to check lowercase vs uppercase
I am on macOS Sonoma 14.4.
ok, thanks. @fedorov @pieper would be possible, when you have time, to perfom the same test that I did in DICOM database update is requested after new Slicer version, but appears to be impossible - #11 by Davide_Punzo on MacOS?
the idea would be to understand if it an issue at OS level (i.e. performing the same test I did on linux) or if it is something related to the database that Andrey tried to update (size, type of DICOM, version of the previous schema, etc…).
Finally, Andrey do you still have a copy of the database before updating it? and is it something that you can share?
@pieper I think the issue is this commit:
I am not sure 100%, it could be a mix of things, but my investigation points to that. This because the count display works only up to Slicer version 5.6.2 which uses the dicom schema 0.7.0, i.e just before the linked commit which modifies the lower/upper case tags stuff too.
Adding .toUpper() in CTK/Libs/DICOM/Core/ctkDICOMDisplayedFieldGeneratorAbstractRule.h at 7ed1da357b9e7e2462b4b764882612484c6fa051 · commontk/CTK · GitHub :
/// Utility function to convert a DICOM tag enum to string
static QString dicomTagToString(const DcmTagKey& tag)
{
return QString("%1,%2").arg(tag.getGroup(),4,16,QLatin1Char('0')).arg(tag.getElement(),4,16,QLatin1Char('0')).toUpper();
}
fix the issue, I have created a PR in CTK (BUG: Convert lower case dicom tags by Punzo · Pull Request #1203 · commontk/CTK · GitHub). But I am not sure if we need to apply the fix in any another part of CTK
Ah, good catch Davide - yes, this mix of upper and lower case hex tags was a big pain. I think we need to be very consistent and use upper case everywhere.
Ah, good catch Davide - yes, this mix of upper and lower case hex tags was a big pain. I think we need to be very consistent and use upper case everywhere.
I agree. I had a look and I think the only missing one was this one.