Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] load_data: switch to binary mode for recent versions #394

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

jue-adhoc
Copy link
Contributor

We found the following error when trying to upload csv files via the load_data method:

Traceback (most recent call last):
  File "/home/odoo/src/openupgradelib/openupgradelib/openupgrade.py", line 2304, in wrapped_function
    func(
  File "/home/odoo/src/repositories/ingadhoc-odoo-saas-adhoc/saas_client_l10n_ar/migrations/16.0.1.9.0/post-migration.py", line 12, in migrate
    openupgrade.load_data(env.cr, 'l10n_ar', 'data/res.country.csv')
  File "/home/odoo/src/openupgradelib/openupgradelib/openupgrade.py", line 353, in load_data
    tools.convert_csv_import(
  File "/home/odoo/src/odoo/odoo/tools/convert.py", line 780, in convert_csv_import
    reader = pycompat.csv_reader(io.BytesIO(csvcontent), quotechar='"', delimiter=',')
TypeError: a bytes-like object is required, not 'str'
2024-12-13 17:27:20,327 75 WARNING test-bidargentinasa-13-12-2 odoo.modules.loading: Transient module states were reset 
2024-12-13 17:27:20,328 75 ERROR test-bidargentinasa-13-12-2 odoo.modules.registry: Failed to load registry 
Traceback (most recent call last):
  File "/home/odoo/src/odoo/odoo/modules/registry.py", line 91, in new
    odoo.modules.load_modules(registry, force_demo, status, update_module)
  File "/home/odoo/src/odoo/odoo/modules/loading.py", line 484, in load_modules
    processed_modules += load_marked_modules(cr, graph,
  File "/home/odoo/src/odoo/odoo/modules/loading.py", line 372, in load_marked_modules
    loaded, processed = load_module_graph(
  File "/home/odoo/src/odoo/odoo/modules/loading.py", line 236, in load_module_graph
    migrations.migrate_module(package, 'post')
  File "/home/odoo/src/odoo/odoo/modules/migration.py", line 189, in migrate_module
    migrate(self.cr, installed_version)
  File "/home/odoo/src/openupgradelib/openupgradelib/openupgrade.py", line 2304, in wrapped_function
    func(
  File "/home/odoo/src/repositories/ingadhoc-odoo-saas-adhoc/saas_client_l10n_ar/migrations/16.0.1.9.0/post-migration.py", line 12, in migrate
    openupgrade.load_data(env.cr, 'l10n_ar', 'data/res.country.csv')
  File "/home/odoo/src/openupgradelib/openupgradelib/openupgrade.py", line 353, in load_data
    tools.convert_csv_import(
  File "/home/odoo/src/odoo/odoo/tools/convert.py", line 780, in convert_csv_import
    reader = pycompat.csv_reader(io.BytesIO(csvcontent), quotechar='"', delimiter=',')
TypeError: a bytes-like object is required, not 'str'
2024-12-13 17:27:20,329 75 WARNING <databasename> odoo.addons.saas_client.cli.fixdb: Could not fix dbs, this is what we get a bytes-like object is required, not 'str' 

This is because it is expecting a binary file and it is receiving a str.

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, as of version 11.0 Odoo uses BytesIO not StringIO and opens the file with mode rb not b (https://github.com/odoo/odoo/blob/11.0/odoo/tools/convert.py#L784). It is not the same in earlier versions. Can you make this behaviour version dependent, and please fix the pre-commit.

@StefanRijnhart
Copy link
Member

Oh, and please change commit message to include the method name (rather like the module name in other repos), e.g. [FIX] load_data: switch to binary mode for recent versions

@pedrobaeza
Copy link
Member

Is this affecting XML files loading?

@StefanRijnhart
Copy link
Member

StefanRijnhart commented Dec 13, 2024

Is this affecting XML files loading?

As far as I can tell, Odoo loads all file types with mode rb as of Odoo 11.0: https://github.com/odoo/odoo/blob/11.0/odoo/tools/convert.py#L784-L796 (and this is still the case in Odoo 18), but convert_xml_import then fetches the filename from the open file handle and opens the file again.
So it should not affect XML file loading.

@zaoral
Copy link
Contributor

zaoral commented Dec 14, 2024

@StefanRijnhart
Indeed, we follow with @jue-adhoc this approach because of that exact code you mention in the last message.
Also, we tested the import both csv and xml files and it was working properly @pedrobaeza
If you think will be better to add a unit test for this please let us know

Best Regards

@pedrobaeza
Copy link
Member

OK, then rename the commit as stated by Stefan.

@jue-adhoc
Copy link
Contributor Author

jue-adhoc commented Dec 16, 2024

Oh, and please change commit message to include the method name (rather like the module name in other repos), e.g. [FIX] load_data: switch to binary mode for recent versions

Hi @StefanRijnhart / @pedrobaeza , thank you for your review. The commit message is already updated.

@pedrobaeza
Copy link
Member

Please check pre-commit.

@StefanRijnhart
Copy link
Member

@jue-adhoc Please also make the change version specific. This code needs to remain compatible with older versions of Odoo, so only apply the change when the version is 11 or higher.

@jue-adhoc
Copy link
Contributor Author

@jue-adhoc Please also make the change version specific. This code needs to remain compatible with older versions of Odoo, so only apply the change when the version is 11 or higher.

@StefanRijnhart sorry, I misunderstood your previous review on this point. I already made the changes to make the fix compatible with versions older than 11. Thank you!

@StefanRijnhart StefanRijnhart changed the title [FIX]: fix error for reading csv files [FIX] load_data: switch to binary mode for recent versions Dec 17, 2024
Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

@pedrobaeza pedrobaeza merged commit f010fdd into OCA:master Dec 17, 2024
2 checks passed
@zaoral zaoral deleted the 84292-jue branch December 19, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants