search

craftcms / commerce

Fully integrated ecommerce for Craft CMS.
https://craftcms.com/commerce
Other
217 stars 170 forks source link

[4.x]: Invalid ownerId in addresses table after upgrade from 3.x #2939

Closed rob-baker-ar closed 1 year ago

rob-baker-ar commented 2 years ago

What happened?

Starting versions:

Craft: Craft Pro 3.7.51 Commerce: 3.4.16

Description

After the steps described in "Steps to reproduce", running this query directly on the database:

-- Note: many rows returned with a `null` value for `ownerId`.
SELECT * FROM addresses WHERE countryCode='GB';

This might be a clue to an issue when running a similar query from a PHP / Craft context:

$addresses = Address::find()->countryCode('GB')->all();

Exception thrown is:

Exception: Invalid owner ID: 3890 (/[...]/vendor/craftcms/cms/src/elements/Address.php:301)

Steps to reproduce

  1. setting new versions in composer.json
  2. run: composer upgrade
  3. run: craft clear-caches/all
  4. run: craft cache/purge-all
  5. run: craft migrate/all
  6. run: craft commerce/upgrade

In the last step, all defaults were selected, it's output:

Ensuring we have all the required custom fields…
Customer and order addresses will be migrated to native Craft address elements.
Some of the existing addresses contain data that will need to be stored in custom fields:
 - Attention
 - Title
 - Business ID
 - Phone Number
 - Alternative Phone
Do you have a custom field for storing Attention values? (yes|no) [no]:
Let’s create one then.
Field handle: [addressAttention]
Field name: [Attention]
Do you have a custom field for storing Title values? (yes|no) [no]:
Let’s create one then.
Field handle: [addressTitle]
Field name: [Title]
Do you have a custom field for storing Business ID values? (yes|no) [no]:
Let’s create one then.
Field handle: [addressBusinessId]
Field name: [Business ID]
Do you have a custom field for storing Phone Number values? (yes|no) [no]:
Let’s create one then.
Field handle: [addressPhone]
Field name: [Phone Number]
Do you have a custom field for storing Alternative Phone values? (yes|no) [no]:
Let’s create one then.
Field handle: [addressAlternativePhone]
Field name: [Alternative Phone]
Creating a user for every customer…
779 customers migrated.
Done.
Migrating address data…
2765 addresses migrated.
Done.
Updating orders…
Done.
Updating users…
Done.
Updating the store location…
Done.
Updating shipping zones…
5 shipping zones migrated.
Done.
Updating tax zones…
2 tax zones migrated.
Done.
Updating order histories…
Done.
Updating discount uses…
Done.
Cleaning up…
Done.

All the above steps complete successfully - I have their output if needed.

Row counts

commerce_customers count (before commerce/upgrade): 1643564
commerce_addresses count (before commerce/upgrade): 2765
commerce_customers count (after commerce/upgrade): 1161
addresses count (after commerce/upgrade): 2767

As part of the upgrade the number of rows is massively reduced - generally speaking this is probably good - I don't need 1.6m rows in commerce_customers as the huge majority are the result of old carts for customers that never completed an order.

Expected behavior

All rows in addresses table have a valid value for ownerId.

Actual behavior

Something is going wrong during commerce/upgrade command leaving broken data in the addresses table - there are both rows with a null value for ownerId as well as rows that have a integer ownerId that no longer point to it's owner (see the value show in in the exception message).

Craft CMS version

4.2.1.1

Craft Commerce version

4.1.0

PHP version

8.1.2

Operating system and version

Ubuntu 22.04 (Linux 5.15.0-46-generic)

Database type and version

MySQL 5.5.5 (mariadb Ver 15.1 Distrib 10.6.7-MariaDB)

Image driver and version

No response

Installed plugins and versions

rob-baker-ar commented 2 years ago

Database dump sent to support@craftcms.com

martyspain commented 2 years ago

I've also got a large number of addresses with a null value for the ownerId in the database after the upgrade. Some users have an address associated with them but many do not, and I've ended up writing extra code into our platform in the interim to handle the missing addresses. I didn't realise it might have been a bug with the migration so I may not be able to resolve the address issue at this point, but wanted to add that I've seen the same issue that @rob-baker-ar reports.

angrybrad commented 2 years ago

FTR addresses with a null value in the ownerId is expected behavior. An address don't have to belong to a user, it can belong to a store. The exception that @rob-baker-ar is getting is a separate issue.

rob-baker-ar commented 2 years ago

Having tried this a few more times over the last couple of weeks I'm still having the same problem.

A key point I think: The main thrust of this issue is is not null values for ownerId, it's the numeric values in that column that seem to be orphaned from the parent elements table (i.e. no corresponding matching value in that table).

There is a separate issue to do with a first party commerce plugin that is currently preventing me seeing if a garbage collection run will mitigate or fix any of this but I plan to have another go at that this week.

pdaleramirez commented 1 year ago

@rob-baker-ar Does the migration issue resolved when Digital Products is disabled before upgrade?

rob-baker-ar commented 1 year ago

@pdaleramirez With Digital Products disabled, I still get the integrity constraint exception from https://github.com/craftcms/digital-products/issues/75 when trying to run garbage collection before the upgrade (so as an effect the upgrade still runs with ~1.6million customer records).

For the upgrade itself, after doing:

I am still left with the headline issue (numeric ownerId values on the addresses table pointing to elements that don't exist - i.e. still throwing the same exception).

pdaleramirez commented 1 year ago

@rob-baker-ar This issue has been fixed on craftcms/digital-products version 3.1.0. Please open an issue on the craftcms/digital-products if you are still experiencing the issue

rob-baker-ar commented 1 year ago

@pdaleramirez

With Digital Products disabled, I still get the integrity constraint exception from https://github.com/craftcms/digital-products/issues/75

I am still left with the headline issue (numeric ownerId values on the addresses table pointing to elements that don't exist - i.e. still throwing the same exception).

Perhaps I'm missing something: how would the fix to digital products affect rows on the address table that are effectively detached from the elements table? There are foreign key constraints on the addresses table but the ownerIds from the addresses table point to rows that do not exist on elements table.

Looking at this from v3.1.0: https://github.com/craftcms/digital-products/commit/3414823f927be87b4e58e3962d90278f8c95976e craft\digitalproducts\Plugin::_registerGarbageCollection() calls Gc::deletePartialElements() which only affects the products, licenses and elements tables.

pdaleramirez commented 1 year ago

Addresses tables shouldn't be associated with digital product element as owner. Unless it explicitly assigned through custom module or plugin in that case a custom query should be executed to delete the association.

rob-baker-ar commented 1 year ago

@pdaleramirez

Addresses tables shouldn't be associated with digital product element as owner.

I couldn't agree more. We have no code that changes ownerId on addresses in our modules nor have we ever. Put another way: if there are some associations between digital products and addresses, they have not been created by our code. This must have happened as a result of a core or commerce plugin related migration at some point over the last 2-3 years that became apparent after the migration that created the addresses table or as an effect of some other functionality built into the plugin or core.

However, if it is safe to do a delete on the addresses table where the ownerId column has a numeric value who's corresponding row is not in the elements table will that fix this?

pdaleramirez commented 1 year ago

@rob-baker-ar Deleting the row should be fine if the foreign key doesn’t exist.