world icon indicating copy to clipboard operation
world copied to clipboard

Using ISO codes as primary keys

Open elnurvl opened this issue 8 months ago • 12 comments

In my opinion, ISO codes are more appropriate to be used as primary keys for the tables in this repository rather than "randomly" assigned integers. It makes more sense to fetch a country by Country::find('az') instead of dealing with where queries. Same applies for currencies, languages, states and even maybe cities. Plus this would give an extra benefit of indexing the records by the standardized codes by default.

I almost forgot to mention how the foreign IDs would become more self-descriptive.

What do you think?

elnurvl avatar Apr 25 '25 23:04 elnurvl

@elnurvl Honestly i never thought of this. The reason is that i always looked at primary keys as self incremented integers. Regrading the helpers you are looking at, i agree that they will be an important addition to the package. I would suggest to create facades as: Country, Language which can return the entity by ISO2, locale... etc... Is your primary interest a restructure of the DB? We need to maintain the backward compatibility. Or is it having more helpers to make the usage of the package easier?

Please let me know what do you think?

nnjeim avatar Apr 29 '25 08:04 nnjeim

@nnjeim , sorry for the late response. Primary keys as self-incremented integers make sense if the table is meant to filled up with insert queries over the time, because you need to keep generating a unique key for every new record. Here all the records are fixed and inherently come with unique keys.

Facade would not resolve all of the issues. What about project models related to these package models, such as $user->country. These models would need to customize relationship definitions across project models to make sure they are related by codes. Yes, there might be special traits introduced to address these, but what is the benefit of keep sticking with the incremented IDs anyway? Relying on ISO codes across foreign key columns would give an extra benefit of visibility.

I agree this might require releasing a new major version. But hopefully my points make sense to you to support such an initiative. Please let me know.

elnurvl avatar May 15 '25 06:05 elnurvl

@elnurvl i would love to brain storm further more with you on this. Do you have the flexibility to discuss this further more. If yes please suggest how we can connect?

nnjeim avatar May 15 '25 08:05 nnjeim

@nnjeim , I’d prefer to carry the discussion over here so others could also contribute with their opinions and the decision process would be publicly visible. If you still think we should have a call let me know of your timezone.

elnurvl avatar May 15 '25 09:05 elnurvl

i am totally against it. there is a convention in laravel to always use numeric primary key for a table and numeric keys are faster for making joins. also package-users can easily create relationships between their models and this package models. if you would like to have Country::find('az') you can extend package model and override find method.

kminek avatar May 24 '25 20:05 kminek

there is a convention in laravel to always use numeric primary key for a table

@kminek , Laravel has no such a convention. They provide string keys for UUID, ULID-based tables as well out of the box.

numeric keys are faster

This statement does not apply here: for a table of 200 records you cannot tell the difference in performance. Fun (irrelevant) fact: But even for big tables, UUID-based primary keys are preferred even at the cost of performance nowadays, to avoid enumeration attacks.

also package-users can easily create relationships between their models and this package models

Not if the existing databases have records relied on standardized ISO codes: one would need to define relationships with customized foreign IDs all over the codebase. Not convenient, not robust.

you can extend package model and override find method

Now this is DEFINITELY not how you follow Laravel conventions. Besides, it is not about the find method. The issues mainly start with relationships and table visibility.

@nnjeim , have not heard from you regarding this since you expressed your interest on discussing the matter. If this makes sense for you, I can present a PoC in a PR when I get a time.

elnurvl avatar May 28 '25 13:05 elnurvl

@elnurvl Thank you for the time spent to detail your point of view. I would suggest to refactor the code if you would like to and see concretely what advantages are we bringing in. I had cases in the past where i need to relate unrelated models and for this i have created a related table where 2 entries defined by their type and id are related.

Schema::create('relatables', function (Blueprint $table) {
			$table->string('source_type');
			$table->unsignedInteger('source_id');
			$table->string('related_type');
			$table->unsignedInteger('related_id');
		});

it might be used to related in your case a user entity to a country entity, you might think to modify it to relate a user to a country defined by its iso2.

another approach could be to add in user or user_metas the iso2 of the country if the modification of the user table is not an option.

in that case we would simply add a helper to resolve the country by iso2 and potentially cache it for performance gains.

the countries table can be indexed by id and iso2

Schema::create('countries', function (Blueprint $table) {
    $table->id(); // Creates an auto-incrementing primary key index
    //
    $table->string('iso2', 2)->unique(); // indexes ISO2
});

I would simplify the approach by creating a helper wrapping the below

$country = Country::where('iso2', 'FR')->firstOrFail();

public function scopeByIso2($query, $iso2)
{
    return $query->where('iso2', $iso2);
}

$country = Country::byIso2('FR')->first();

nnjeim avatar May 28 '25 14:05 nnjeim

@nnjeim , maintaining another meta table complicates thing even more, doesn't it? It seems you are willing to introduce more components for the sake of keeping the existing table scheme. I would like to know the reason aside from the fact there would be a new major release.

I already listed benefits of the suggested change above. Here are the recap:

  1. No need to rely on custom-defined obscure numbers when attaching countries/currencies to other models. No need for an extra query to obtain these IDs from the ISO codes.
  2. No need for a translation layer between clients and the server (clients have no idea what is country 104 and it should not).
  3. When you look at the raw table(i.e. users), you would have a better visibility on the related data. You would not have to guess which country is corresponding to the number shown in the column.

The question here is what is the benefit of numbered primary keys over the ISO-matching ones?

  1. Theoretically faster.

I could not think of another reason for keeping the existing design. But again, with tables of predefined (very small) data I don't understand how this is a relevant statement.

elnurvl avatar May 28 '25 17:05 elnurvl

@elnurvl you can always fork the package and introduce your changes there for users to test this approach

kminek avatar May 28 '25 18:05 kminek

@elnurvl Thank you again for your input.

Could you please elaborate on the following points:

  • Why there’s no need for an extra query to obtain these IDs from the ISO codes.
  • How examining the raw table (i.e. the users table) provides better visibility into the related data.

Fundamentally, we’re asking: how can we relate a user to a country? Couldn’t we simply store the ISO2 code as user meta—either directly on the users table or in a separate user_metas table?

I believe the package should remain decoupled from any business logic related to users, companies, branches, or other consuming modules.

For example:

if you install it in a monolithic application, the World facade could render any country like this:

World::countries([
    'fields'  => 'states,cities',
    'filters' => [
        'iso2' => 'FR',
    ],
]);

If the package is installed in an API, i would query:

https://world.bmbc.cloud/api/countries?filters[iso2]=FR&fields=states,cities

i don't see the need to use the country ID in your user table, all what is needed is the iso2.

All of those queries would be cached, so no additional database calls are needed.

Check by yourself the query above the response time is only 20ms (cached response)

Please forgive me if I’m still missing the points you’re highlighting—looking forward to your reply.

nnjeim avatar May 29 '25 05:05 nnjeim

@nnjeim , before I repeatedly go over the same points, I would also like to know your perspective on how the existing design is better and why we are resisting to change it at the cost of forcing consumers to invent workarounds.

Here are my perspective, once again:

Why there’s no need for an extra query to obtain these IDs from the ISO codes.

Because the IDs become the ISO codes. When you want to assign a country to any model, you just put what was given in the request body(ISO code) in

Car::create(['country_code' => $request->input('country_code')])

whereas in the current state you need to do:

$countryId = Countries::firstWhere(['country_code' => $request->input('country_code')])->id;
Car::create(['country_code' => $countryId])

How examining the raw table (i.e. the users table) provides better visibility into the related data.

| id | name        | country_code |
| 1  | John Doe |  de                    |

vs

| id | name        | country_id       |
| 1  | John Doe |  67                    |

So in which table you can get an idea of the user's country? Did you figure out 67 corresponds to Germany right away? Or it was more intuitive to see de?

Couldn’t we simply store the ISO2 code as user meta—either directly on the users table or in a separate user_metas table?

simply you said? :) I don't see how it is a simple solution. Was not the idea of having this package to free the developers from the pain of implementing their own logic for such features?

i don't see the need to use the country ID in your user table, all what is needed is the iso2.

I don't know, man. I believe I already explained my points several times already. In conclusion, assigning numbers to countries and currencies is a bad practice. I lived it in the experience and I know how it makes the design ugly/error-prone in a big project. If the above points still do not make any sense for you, maybe I should fork the repo as @kminek suggested and continue developing on top of that.

elnurvl avatar May 29 '25 07:05 elnurvl

@elnurvl The below applies for countries iso2 and currencies as well.

I am assuming that in your UI you have an index method rendering all the countries {id, name and iso2}[].

[
   {
     id: 123,
    country_name: 'France',
    iso2: 'FR'
  },
.
.
.
]

When you i persist an entity in the db as Car in your case you can persist the iso2

Car::create(['country_code' => $request->input('iso2')])

I truly don't see the sense into drifting the database design from what is the common practice.

Again we can create helpers as get country by iso2 as an additional query if needed.

Changing the db structure is a breaking change and wouldn't be a smooth transition for existing users.

After all, i respect your opinion and looking forward to test a fork where you implement the changes.

nnjeim avatar May 30 '25 06:05 nnjeim