-
Notifications
You must be signed in to change notification settings - Fork 64
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
Migrate table names with uppercase to lowercase names (macOS, Windows) #6333
base: main
Are you sure you want to change the base?
Conversation
The review is in German as I don't want that translations errors are resulting in discussions which are not needed.
|
Zu 1. Und was folgt daraus jetzt? Code, der mit Claude.AI erzeugt wurde, darf frei verwendet werden. Die Verantwortung, dass er korrekt ist, trägt man selbst. Das ist nicht anders als bisher, wo man häufig Codeteile durch "Googeln" zusammengesucht hat. Zu 2. Hast du es ausprobiert? Warum sollte der Code z. B. unter Linux nicht funktionieren? Zu 3. Behandelt werden alle Objekte in |
SELECT TABLE_NAME | ||
FROM INFORMATION_SCHEMA.TABLES | ||
WHERE TABLE_SCHEMA = DATABASE() | ||
AND TABLE_NAME != LOWER(TABLE_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hier kann man – wenn dies tatsächlich erforderlich ist – alternativ oder als weitere Bedingung ergänzen, dass nur ausgewählte Tabellen umbenannt werden: AND TABLE_NAME IN ('Tabelle1', 'Tabelle2', ...)
(siehe Diskussionsbeitrag, wo angemerkt wird, dass die Kitodo-Datenbank auch Tabellen enthalten kann, die nicht zu Kitodo gehören). Ebenfalls möglich ist die Beschränkung auf Tabellen mit AND TABLE_TYPE = 'BASE TABLE'
.
Ja, dass habe ich und sonst hätte ich es nicht geschrieben. Da wir an der SLUB von dieser Änderung betroffen wären. Ich zeige aber mit Absicht nicht den Fehler auf, warum dies nicht funktioniert.
Das ist keine korrekte Aussage. Alle bisherigen Migrationsskripte haben nur die gewählten / explizit genannten Tabellen der Anwendung migriert und nicht pauschal alle, wie es hier in diesem Fall vorliegt.
Dazu gibt es keine Aussage bisher und bis zu diesem PR war dies auch nicht notwendig, eine solche Aussage zu treffen, da jeweils nur die anwendungsspezifischen Tabellen geändert wurden und nicht pauschal alles. Dies jetzt in Frage zu stellen, halte ich für falsch und ist auch nicht zwingend notwendig
Das ist deine Ansicht, ich sehe bisher keine Gründe, warum dies verboten sein sollte. Das die Pflege von anwendungsfremden Tabellen, Views und Procedures bei jeden selbst liegt, steht gar nicht zur Diskussion. Jedoch eine Änderung bzw. Anpassung dieser Tabellen, Views und Procedures sollte nicht durch die Migrationsskripte der Anwendung passieren sondern durch die jeweiligen Administratoren selbst. Nur diese wissen, wie die Anpassungen durchzuführen sind.
Da dieser PR wohl auch bei Systemen eingesetzt werden soll, die Groß- und Kleinschreibung unterscheiden und wo es vermutlich nicht einmal notwendig ist, kommt es erst durch diesen PR zu Problemen, wenn anwendungsfremde Tabellen, Views und Procedures umbenannt werden. Ich erwarte von einer Migration, dass die anwendungsspezifschen Daten geändert werden aber beim besten Willen keine anwendungsfremden Daten. |
Anmerkung 2. ist jetzt korrigiert, und bezüglich 3. habe ich auf "BASE TABLES" eingeschränkt, so dass andere Objekte nicht berührt werden. Haben wir eine Liste aller Tabellen, die behandelt werden sollen? Dann könnte ich diese noch ergänzen. |
Prinzipiell könnte jede bestehende Tabelle der Anwendung betroffen sein, jedoch ist die Anzahl deutlich geringer und diese sind doch in #4412 genannt. Als weitere Quellen könnten #6139 , #4412 und #3998 dienen. |
Mit dem letzten Commit wird die Umbenennung auf die fünf bekannten Tabellen beschränkt, die unter macOS sonst einen Namen mit Großbuchstaben hätten. |
For case sensitive filesystems which are typically used by Linux such renames were already done in earlier migration steps, but those steps have no effect on macOS and Windows. The new SQL migration code renames any table name with uppercase letters to a lowercase table name. The script was created by Claude.AI. Fixes: kitodo#6139 Signed-off-by: Stefan Weil <[email protected]>
The added `BINARY` makes the condition case-sensitive. Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Signed-off-by: Stefan Weil <[email protected]>
Die Verwendung von KI ist eine interessante Frage. Ich denke, dass man als Programmierer auch Hilfe von KI in Anspruch nehmen kann, aber es bleibt die Verantwortung des Programmierers, der die Commits erstellt, zu verstehen, was sie tun, und es liegt in seiner Verantwortung, dass sie das tun, was sie tun sollen. Wenn ein Bug darin ist, muss man immer noch verstehen können, warum, um ihn beheben zu können. Aber es kann hilfreich sein, dass die KI möglicherweise eine Syntax verwendet, die man nicht kannte, und einen dabei unterstützt, die richtigen Anweisungen zu finden, und zu verstehen, wie man sie nutzen kann. Übrigens würde ich es vermeiden, eine Prozedur zu definieren und sie später zu löschen, und lieber nur die einfachen Anweisungen aufschreiben. Wenn die Ausführung des SQL aus irgendeinem Grund abbricht, bleibt die Prozedur für immer in der Datenbank, und Jahre später, wenn ein Server migriert wird, fragt sich jemand, wofür sie gebraucht wird. Ich frage mich, warum deine Datenbank überhaupt noch Tabellen mit Großbuchstaben enthält, meine nicht (und das entspricht den Annotationen in den Bean-Klassen):
Kann die Datenbank Die Copyright-Frage kann ich überhaupt nicht beurteilen. (Bezüglich der Groß- und Kleinschreibung möchte ich hinzufügen, dass der Windows Explorer seine Anzeige manchmal nicht richtig aktualisiert, wenn sich nur die Groß- und Kleinschreibung eines Dateinamens ändert. Der Dateiname wird auf der Festplatte geändert. Das ist hier ab vom Thema, aber ich erwähne es, da es in dem Kontext eine Quelle für Verwirrung sein kann.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check again whether there are any tables with capital letters in the database at all. In my case, this is not the case. Then, this migration would at least be superfluous (and perhaps better in the kitodo-contrib
repository).
Das ist momentan der Normalfall auf macOS, wenn man dort Kitodo neu installiert (siehe CI Demo). Gleiches gilt vermutlich für Windows (abhängig davon, wie dort mariaDB läuft). Und bevor jetzt jemand einwendet, dass weder macOS noch Windows die unterstützten Laufzeitumgebungen sind: ich entwickle unter macOS, und da ist es einfach schön, wenn das ohne spezielle Klimmzüge funktioniert. |
Die aktuelle Fassung meines Commits berücksichtigt das und gibt die fünf Tabellen, deren Namen zu korrigieren sind, explizit an. |
Würde dieser Migrationsschritt auch nach einem (unwahrscheinlichen) Abbruch als "durchgeführt" registriert werden? Oder würde spätestens die nächste Migration mit flyway den Schritt wiederholen und dabei die Prozedur entfernen? Unabhängig davon ist der Name der Prozedur "rename_tables_to_lowercase" ziemlich eindeutig. |
For case sensitive filesystems which are typically used by Linux such renames were already done in earlier migration steps, but those steps have no effect on macOS and Windows.
The new SQL migration code renames any table name with uppercase letters to a lowercase table name.
The script was created by Claude.AI.
Fixes: #6139