[FIX] base_location: relax sql constraint

[wtaferner]

In Austria we have several zipcodes for the same name and therefore this constraint would make this module unusable...

Not sure though why it was introduced like it is, but I hope it was just an oversight.

Info: @wt-io-it

[pedrobaeza]

It was on purpose. Are you saying that you have in the same state cities with the exact same name?

[wtaferner]

Jep, we do have the same name with serveral zip codes.

[pedrobaeza]

But then use the same city and put those different zipcodes inside the city.

[pedrobaeza]

The problem not doing this way is the denormalized data structure if not done this way. The layout is:

• Country > State > City > Zip codes. If city is not unique for the same upper levels, then you won't be able to determine the city given its name.

[wtaferner]

Hmm, but Odoo Core has the zipcode in res.city, so it is always kinda redundant and therefore wrong data if I abstract it like you suggest, especially if you want to move back and forth where you loose data or need to migrate with complexity by eliminating base_location.

Is there a reason why zipcode is not part of the constraint?

[pedrobaeza]

This module changes the core structure to follow the one listed above, as we want to have an unique entity city, valid for other data models and behaviors. The Odoo city is not a city in fact.

[wtaferner]

I got that, but I don't get why we can't leave it logically as it is and extend it for our purpose...and if someone is not filling up zipcode (blank) it is basically the same constraint, but maybe I need to study the behavior in 13.0 in detail to understand first what we still do after Odoo introduced base_address_city.

I will come back with more insights if you don't turn it down already yet 😉
Thank you in the meantime 😃 Hope you are doing well.

[pedrobaeza]

With this module, zipcode is basically useless, so I'm afraid although we change this constraint, it won't serve you, but I'll wait more insights.

[wtaferner]

Alright Pedro, I think I have now the full insight and I do understand that zipcode is not used and even invisible to the user.
The concept of base_location suggests to put all zips for a certain name, state_id, country_id in res.city.zip which is clear and straight forward.

Nevertheless, I would think it is no mistake to relax the constraint to zipcode too as it won't hurt the standard use case for the user (null value of zipcode will enforce the same behavior), but additionally will keep the compatibility with upstream intact whereas if someone needs to adapt its data files to the new base_location constraint it destroys this compatibility (possible 1:1 relation instead of 1:n for the same name) + put extra work on the shoulders to migrate this data (one-way into direction of base_location as it adds this extra layer and enforces it too early IMO as I guess/hope that Odoo core might "follow" as they did several times before).

I know that you are straight with your opinions but maybe you could consider the PoV of the ones like me who want to keep it in line with upstream data (possibility to move back and forth without pain) and do not keep and enforce this breaking constraint.

Maybe there are others who want to follow my argument and back it here, but let me say: "SQL constraints suck anyway" 😜

[Yenthe666]

Just my five cents: I'd prefer the solution from Wolfgang too. The restriction is still pretty good and it solves the issue with cities that have the same same.

[pedrobaeza]

Well, I still don't understand if this module hides all that part, why do you still wants to relax in it the constraint. It's fighting against the current. And if you plan to extend it someway, do you know that you can inherit the SQL constraint for changing it?

[wtaferner]

And if you plan to extend it someway, do you know that you can inherit the SQL constraint for changing it?

Inherit by overriding? I guess so, but I was probably just triggered that this measure is/was too extreme and I understand the idea behind enforcing the user itself.

So yeah, no issue with this not being merged besides the arguments I have written to consider. It always depends on what you want to enforce, when and to whom 😉

Maximum technical flexibility would be my target...ensure 1:1 would be possible if you clearly define the zipcode on the res.city instead of enforcing 1:n and loosing the maybe structured information in Odoo core (from some data files to be maintained along the road)

[pedrobaeza]

Well, if others reviewers decide that the constraint should be changed, I'm not blocking. Anyway, this PR is red, but I haven't checked why.

[wtaferner]

@pedrobaeza
Thx. I fixed the tests by adding an empty string value to zipcode as null would not work as part of the relaxed/extended constraint interestingly. This basically also probably simulates the reality even better as the form view would probably also send an empty string to the backend as long as it is not readonly. TIL a bit more about sql constraints in terms of null 😄

[pedrobaeza]

NULL is not the same as empty string "" and ORM put NULL there, so I'm afraid this is going to be conflicting when not using zipcodes.

[wtaferner]

NULL is not the same as empty string "" and ORM put NULL there, so I'm afraid this is going to be conflicting when not using zipcodes.

Sorry, but the test say something else otherwise I would not have gone down this road...

[pedrobaeza]

Well, I'm just pointing the problem by my experience on that, and seeing that you need to explicitly set the field with "" instead of accepting the default null.

[wtaferner]

If we want to avoid that we could set a default="" which was my first approach. Anyway, this will not be merged anyway I feel, so better spare the time, right? @alexis-via did even fork as the whole res.city.zip seems redundant.
I gave it a try and failed here which is ok. It needs two approvals and I have not even one yet :-)

[pedrobaeza]

I have already explained why res.city.zip is there for having a normalized data model, but if you think its enough with base_address_city, then you can stay with such module as Alexis has done. I don't have problem in accepting this change if I'm sure that it won't cause any problem with the approach for which is has been created, and I see grey areas in the changes you have made in the tests and thus I have arisen them.

[wtaferner]

As said, the ORM does not make "" to null and based on this it should work...it does not work if it is null, so I can imagine to set the default that it will never happen that it will become null somewhere.

If you agree I'll do that and we get it done...

[pedrobaeza]

And what happen with existing records with NULL?

[wtaferner]

The constraint can't be set...this is what Odoo does afaik.

EDIT: but true, that is a reasonable question...damn it 🤣

[pedrobaeza]

But then it will break all the existing installations... That's why I'm not liking too much to add this. Adding a migration script that turns NULL to "" will avoid it initially, but I'm not sure if all the existing modules that feed these models will do it correctly.

[wtaferner]

Is there an upgrade hook we could use?

[pedrobaeza]

Yes, migration scripts can handle it. More or less this code:

env.cr.execute("UPDATE res_city SET zipcode = '' WHERE zipcode IS NULL")

[NL66278]

What is the problem with just leaving zip in the model NULL? That is not change anything in the module. Lots of things will break if you are forcing all existing code to fill a value, even if it is an empty string, in zip. As shown by your need to adapt the tests.

[NL66278]

I set Request Changes, but actually I think this PR should be closed. It is simply not a good idea to add a column that can be NULL to an SQL constraint.

[wtaferner]

@NL66278
Ok, so, what is your change request above what I suggested?
I can offer two things, adding default="" to not have NULL by accident and adding required=True that NULL will not have the possibility to land in the database.

@pedrobaeza
Can you guide me where to put the SQL that on upgrade of the module it is executed? Migration scripts are only executed on migration, but as we are in stable it should be initiated on upgrade, right?

[NL66278]

@wtaferner Adding extra required / NOT NULL fields in an inheriting module will definitely break a lot of stuff and basically should never be done. I simply do not see any advantage in implementing the changes you are proposing.

[pedrobaeza]

At least as informative purposes: @wtaferner you can put migration scripts in any version inside the Odoo major version for doing data massive changes. The upgrade engine is part of Odoo itself (it's not something of OpenUpgrade). Check this illustrative example: OCA/credit-control#86

[wtaferner]

@NL66278
Sorry, but I am pretty sure if you try to impose NOT NULL in database via upgrade Odoo will deny to set it in database and give a warning, so nothing is broken if the requirements are not given for that and basically the default will avoid having any NULL value and also would make the SQL constraint effective as NULL is destroying the constraint as intended.
If the zipcode would be an empty string it can support both worlds (with only 1:n and with a 1:1 relation and therefore an explicit zipcode)

Anyway, I would appreciate, if you could have given an example more explicit than "break a lot of stuff" and "should never be done", but I do agree that it is too heavy for a stable version and others, so I close the PR and move on.

@pedrobaeza
Thank you for the hint. It is not 100% clear from the example, but I looked up the migration manager in Odoo Core and there my question was kinda answered in the doc strings and there is quite some helpful stuff I did not look up before. Great to have this PR done as I learned some things 😄
As Odoo S.A. is denying to open source the migration scripts for an open community effort I will probably join step by step the openupgrade initiative to learn and contribute from there besides the daily insanity helping people to implement Odoo in their companies and becoming more efficient.

Thank you for all those who spent their valuable time for this PR too. Much appreciated.