agritheory / test_utils

Common Test Utilities and Fixtures for AgriTheory Projects
2 stars 3 forks source link

customize.py should opt out if `migration_hash` is the same (no changes detected) #17

Closed agritheory closed 2 months ago

MyuddinKhatri commented 2 months ago

@agritheory During migration, Frappe checks the migration hash for standard DocTypes. If there are any changes in the JSON file, it returns a different hash and then syncs the changes accordingly. https://github.com/frappe/frappe/blob/d1f5e14e8e18b1e2b9c80fdf7bf567fc663b7202/frappe/modules/import_file.py#L141

However, when I used frappe.db.get_value("DocType", "BOM Scrap Item", "migration_hash"), it returned the same value as calculate_hash("/apps/erpnext/erpnext/manufacturing/doctype/bom_scrap_item/bom_scrap_item.json") (without any changes in the file). After I changed the standard bom_scrap_item.json, calculate_hash returned a different value. Upon running bench migrate, the changes got synced and both hashes matched.

The hash for our customized file and the database value will always differ. I didn't find anything that stores the hash for the customized DocTypes. calculate_hash("apps/beam/beam/beam/custom/bom_scrap_item.json") imported caluculate_hash from https://github.com/frappe/frappe/blob/d1f5e14e8e18b1e2b9c80fdf7bf567fc663b7202/frappe/modules/import_file.py#L14 I am not sure how should we proceed to check for migration_hash for customized doctypes. I would like to know your thoughts on this.

agritheory commented 2 months ago

@MyuddinKhatri Summarizing our conversation from the call: We want to reimplement the hashing Frappe does but extend it to all custom/*.json files for that doctype for all applications. This will detect if there's been a deterministic change in schema based on this static analysis and not migrate if there's no change but there have been user-applied customizations. I think its appropriate to add our own column to the database the same way that Frappe does but with a different name (customization_hash).

MyuddinKhatri commented 2 months ago

@MyuddinKhatri Summarizing our conversation from the call:

We want to reimplement the hashing Frappe does but extend it to all custom/*.json files for that doctype for all applications. This will detect if there's been a deterministic change in schema based on this static analysis and not migrate if there's no change but there have been user-applied customizations. I think its appropriate to add our own column to the database the same way that Frappe does but with a different name (customization_hash).

How should we handle if same doctype is customized by multiple apps? Should we include custom app name in the column?

agritheory commented 2 months ago

How should we handle if same doctype is customized by multiple apps? Should we include custom app name in the column?

Compute the hash on multiple files as concatenated text. This has an outside chance of triggering a migration due to whitespace, but I think that's acceptable.

MyuddinKhatri commented 2 months ago

How should we handle if same doctype is customized by multiple apps? Should we include custom app name in the column?

Compute the hash on multiple files as concatenated text. This has an outside chance of triggering a migration due to whitespace, but I think that's acceptable.

Okay, we can implement this.

agritheory commented 2 months ago

Some examples:

1

We should reapply our customizations. If we don't, Frappe's version of the schema will be the only one that gets applied and we loose our changes to address_type. This is why we need to include the doctype/doctype.json file in the hash that computes a change in schema.

2

We need to reapply all customizations from all apps when we detect there's a customization that has changed. Frappe doesn't do this, it only applies the customizations from the last app in installed apps/ "hooks resolution order". We should respect the installed apps order to give the customizations the correct priority.