Core/Creatures: apply consistent field names to creature_template and its core counterparts #29988

Ovahlord · 2024-05-14T04:19:21Z

Some of the oldest tables in the database did not age well and have many inconsistent naming styles. This PR will get us one step closer to escaping this mess.

Changes proposed:

all fields will now follow the UpperCamelCase naming standard
fields that reference a DB2 storage will use the DB2 storage name followed with the ending 'ID' (e.g. CreatureTypeID, FactionTemplateID)
changed some field types to match their query packet data counterpart

Tests performed:

tested startup, no regression

… its core counterparts * all fields will now follow the UpperCamelCase naming standard * fields that reference a DB2 storage will use the DB2 storage name followed with the ending 'Id' (e.g. CreatureTypeId, FactionTemplateId) * changed some field types to match their query packet data counterpart

heyitsbench · 2024-05-15T14:59:23Z

sql/updates/world/master/2024_99_99_99_world.sql

+CHANGE COLUMN `faction` `FactionTemplateID` SMALLINT UNSIGNED NOT NULL DEFAULT 0 AFTER `VignetteID`,
+CHANGE COLUMN `npcflag` `NpcFlags` BIGINT UNSIGNED NOT NULL DEFAULT 0 AFTER `FactionTemplateID`,


May I ask why ID would be capitalized in the column name but not Npc?

ID was originally intended to be Id which was changed later because Blizzard uses ID for their packet and db2 fields and to stay in line with the hotfixDB, I resorted to ID.

jackpoz · 2024-05-15T20:23:08Z

sql/updates/world/master/2024_99_99_99_world.sql

@@ -0,0 +1,26 @@
+ALTER TABLE `creature_template` 
+CHANGE COLUMN `entry` `CreatureID` INT UNSIGNED NOT NULL DEFAULT 0 FIRST,


Should this be just ID ? What do other tables use as PK name ?

entry, however, since we use CreatureID in all other cases (Creature text, sparring, gossips) we should just turn it consistent

id / entry is far more common as a convention. It also makes much more sense, we already know what table it belongs to.

Jinnaix · 2024-05-15T21:35:37Z

@trickerer @Rochet2
i dont know how much of your code interacts with the creature_template table - if at all - but this will also affect our 335 branch once its done for master.

stoneharry · 2024-05-16T15:28:15Z

I love the motivation behind this PR. The inconsistent naming is something I have moaned about for a long time, so good on you for taking this work on.

However I strongly believe we should not be handling it like this for 3.3.5. Even for master, we should not be modelling the server DB as a mirror of the client DB.

This is going to cause a huge amount of frustration and pain.

Adding creature as a prefix to almost every column is not helpful. It's in the creature class/domain, we know exactly what data it relates to. It's just more characters to type each time you want to perform any query, and when joining tables it's extra redundant information to include. When we join the table, we give it a name -- so now it's going to be creature.CreatureID rather than just creature.id. When writing frequent queries and working with the data on a day to day basis it's going to cause frequent typos and extra time (it sounds like a trivial complaint, but it quickly adds up). It's already a pain with creature_equip_template which already uses CreatureID.

Also bear in mind case sensitivity. Depending on the OS, it does matter. Now we have to write exact case sensitivity rather than knowing everything is lowercase. Again, I think this complicates things without providing any benefit. Blizzard uses PostgreSQL which does not have the same case sensitivity issues as MySQL.

This is creating a lot of breaking changes. The refactor would be far less impactful if we stuck to the existing naming conventions, i.e: id/entry as the primary key (pick one or the other) and just naming the fields without considering which other client DB it might point to. I have many tools that I have written over the years that are all going to be broken by this change. The uptake is straight-forward but there's no need for it to be so intrusive.

The documentation for the tables is actually quite good. I use it as a reference frequently. It does not make the columns any more clear by making them super verbose.

We have full control to compose and structure the data how we like on the backend. Let's not fall into the trap of copying the DBFilesClient which are only a limited subset that has been transformed from Blizzard's schema.

Master has DB2's and an entirely different schema. As easy as it would make a maintainers life to have the same in both versions, it does not actually make sense. They are completely different.

TL;DR: please reconsider how we approach this, at least for 3.3.5.

edit: There's only a few columns prefixed with creature. My primary problem is with the primary key, it should remain id or entry. The other columns are less contentious, but I still believe we should be avoiding mirroring the client DB.

stoneharry · 2024-05-16T15:46:40Z

sql/updates/world/master/2024_99_99_99_world.sql

+ALTER TABLE `creature_template` 
+CHANGE COLUMN `entry` `CreatureID` INT UNSIGNED NOT NULL DEFAULT 0 FIRST,
+CHANGE COLUMN `name` `Name` MEDIUMTEXT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL AFTER `KillCredit2`,
+CHANGE COLUMN `femaleName` `NameAlt` MEDIUMTEXT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NULL AFTER `Name`,


Why do we pick a short string here and have verbose names everywhere else?

name2 or nameAlternative. Maybe name1 and name2? That would actually make querying easier since name is a reserved word in MySQL, no need to escape anymore.

jackpoz · 2024-05-16T16:47:30Z

@stoneharry a couple of notes unrelated to the pr: columns are case insensitive in mysql, and joins with tables should always use aliases, like JOIN creature_equip_template AS cet

stoneharry · 2024-05-16T16:52:22Z

@stoneharry a couple of notes unrelated to the pr: columns are case insensitive in mysql, and joins with tables should always use aliases, like JOIN creature_equip_template AS cet

SELECT ... FROM creature_template creature
JOIN creature_equip_template equip ON creature.CreatureID = equip.CreatureID

vs

SELECT ... FROM creature_template creature
JOIN creature_equip_template equip ON creature.id = equip.id

I think the latter is much better.

jackpoz · 2024-05-16T17:14:04Z

That should be

SELECT ... FROM creature_template creature
JOIN creature_equip_template equip ON creature.ID = equip.CreatureID

I actually prefer this:

SELECT ... FROM creature_template AS c
JOIN creature_equip_template AS cet ON c.ID = cet.CreatureID

stoneharry · 2024-05-16T17:22:53Z

My point was if we are standardising the column names, we should be using id not CreatureID.

What you call your tables when joining is irrelevant.

stoneharry · 2024-05-16T17:32:10Z

Maybe a simpler example is:

SELECT * FROM creature_template WHERE CreatureID = 10

We already know we are targeting the creature_template table, it should just be ID.

If we really want to be pedantic, maybe this conflicts with creature ID.

SELECT * FROM creature WHERE CreatureID = 1;
SELECT * FROM creature_template WHERE CreatureTemplateId = 10;

That would be even more horrendous.

jackpoz · 2024-05-16T17:46:25Z

CreatureID in creature_template table as PK is just wrong, PKs should not have the table name in them.
Which is what I wrote in my example above that I repeat here:

SELECT ... FROM creature_template creature
JOIN creature_equip_template equip ON creature.ID = equip.CreatureID

creature has ID, foreign table creature_equip_template has CreatureID (should be CreatureTemplateID even)

stoneharry · 2024-05-16T18:10:49Z

CreatureID in creature_template table as PK is just wrong, PKs should not have the table name in them. Which is what I wrote in my example above that I repeat here:
SELECT ... FROM creature_template creature
JOIN creature_equip_template equip ON creature.ID = equip.CreatureID
creature has ID, foreign table creature_equip_template has CreatureID (should be CreatureTemplateID even)

This PR proposes renaming the PK to CreatureID, which is what my samples are trying to show the problems with.

Likewise, creature_equip_template should just be id. Yes, it's a foreign key. But it still belongs to creature, so it's obvious what it refers to. It's also a primary key. There would be far less typos/confusion if we had consistency here. I feel like every time I join creature_equip_template I get it wrong first try.

github-actions bot added the Branch-master label May 14, 2024

Ovahlord added 3 commits May 14, 2024 22:42

added conversion query

ebe2950

fixed startup

287ce60

Ovahlord force-pushed the creatureTemplate branch from 69ccc7b to 287ce60 Compare May 14, 2024 21:16

Ovahlord marked this pull request as ready for review May 14, 2024 21:17

Ovahlord added 3 commits May 15, 2024 00:37

renamed Id to ID because a stupid multi-billion company said so

3e24074

fixed db query

70fd419

codestyle fixes

e42e9d9

Frostshake mentioned this pull request May 15, 2024

Update table columns to match latest TrinityCore Frostshake/TrinityCoreClassic#25

Open

heyitsbench reviewed May 15, 2024

View reviewed changes

jackpoz reviewed May 15, 2024

View reviewed changes

stoneharry reviewed May 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core/Creatures: apply consistent field names to creature_template and its core counterparts #29988

Core/Creatures: apply consistent field names to creature_template and its core counterparts #29988

Ovahlord commented May 14, 2024 •

edited

heyitsbench May 15, 2024

Ovahlord May 15, 2024

jackpoz May 15, 2024

Ovahlord May 15, 2024

stoneharry May 16, 2024

Jinnaix commented May 15, 2024

stoneharry commented May 16, 2024 •

edited

stoneharry May 16, 2024

jackpoz commented May 16, 2024

stoneharry commented May 16, 2024

jackpoz commented May 16, 2024

stoneharry commented May 16, 2024

stoneharry commented May 16, 2024 •

edited

jackpoz commented May 16, 2024

stoneharry commented May 16, 2024

		CHANGE COLUMN `faction` `FactionTemplateID` SMALLINT UNSIGNED NOT NULL DEFAULT 0 AFTER `VignetteID`,
		CHANGE COLUMN `npcflag` `NpcFlags` BIGINT UNSIGNED NOT NULL DEFAULT 0 AFTER `FactionTemplateID`,

		@@ -0,0 +1,26 @@
		ALTER TABLE `creature_template`
		CHANGE COLUMN `entry` `CreatureID` INT UNSIGNED NOT NULL DEFAULT 0 FIRST,

Core/Creatures: apply consistent field names to creature_template and its core counterparts #29988

Are you sure you want to change the base?

Core/Creatures: apply consistent field names to creature_template and its core counterparts #29988

Conversation

Ovahlord commented May 14, 2024 • edited

heyitsbench May 15, 2024

Choose a reason for hiding this comment

Ovahlord May 15, 2024

Choose a reason for hiding this comment

jackpoz May 15, 2024

Choose a reason for hiding this comment

Ovahlord May 15, 2024

Choose a reason for hiding this comment

stoneharry May 16, 2024

Choose a reason for hiding this comment

Jinnaix commented May 15, 2024

stoneharry commented May 16, 2024 • edited

stoneharry May 16, 2024

Choose a reason for hiding this comment

jackpoz commented May 16, 2024

stoneharry commented May 16, 2024

jackpoz commented May 16, 2024

stoneharry commented May 16, 2024

stoneharry commented May 16, 2024 • edited

jackpoz commented May 16, 2024

stoneharry commented May 16, 2024

Ovahlord commented May 14, 2024 •

edited

stoneharry commented May 16, 2024 •

edited

stoneharry commented May 16, 2024 •

edited