-
Notifications
You must be signed in to change notification settings - Fork 29
[IMP] export_bg: Enhance data export functionality with chunking and … #343
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
base: 18.0
Are you sure you want to change the base?
Conversation
base_bg/models/base_bg.py
Outdated
| max_retries = kwargs.pop("max_retries", 3) | ||
| name = kwargs.pop("name", f"{self._name}.{method}") | ||
|
|
||
| # Serialize context, converting non-JSON-serializable types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
esto sacalo ya que lo meto en el otro PR
2a6f5ed to
bd9fafe
Compare
bd9fafe to
5dc9236
Compare
…ectly when using a template
5dc9236 to
b8b5e52
Compare
| class Base(models.AbstractModel): | ||
| _inherit = "base" | ||
|
|
||
| def _export_chunk_bg(self, data, export_id, export_format): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dejate algún comentario explicando este método, y también en los métodos de "web_export" y "combine_chunks"
| } | ||
| ) | ||
|
|
||
| if job and not job._get_next_jobs(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acá también, para que quede claro porque hacemos esto
|
|
||
| class IrModel(models.Model): | ||
| _name = "ir.model" | ||
| _inherit = ["ir.model", "base.bg"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
el inherit de base.bg ya no hace falta porque usamos el método de clase
| Model = self.env[params["model"]].with_context(**params.get("context", {})) | ||
| records = Model.browse(params["ids"]) if params.get("ids") else Model.search(params.get("domain", [])) | ||
| Model = self.env[params["model"]] | ||
| if params.get("ids"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ids = params.get("ids")
domain = params.get("domain", [])
records = Model.browse(ids) if ids else Model.search(domain)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Este PR mejora significativamente la funcionalidad de exportación en background mediante la implementación de procesamiento por lotes (chunking) y un sistema de encadenamiento de jobs. La mejora permite manejar exportaciones grandes dividiendo los datos en múltiples jobs que se ejecutan secuencialmente.
Changes:
- Implementación de sistema de batching con campos
batch_keyynext_job_idpara vincular jobs relacionados - Refactorización de la API de enqueue:
bg_enqueueahora delega abg_enqueue_recordsque soporta división por threshold - Nueva lógica de exportación por chunks que genera attachments intermedios y los combina al final
- Cancelación automática de jobs subsiguientes cuando un job del batch falla
- Amplia cobertura de tests para las nuevas funcionalidades de batching
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| base_bg/models/base_bg.py | Refactoriza bg_enqueue para soportar batching con thresholds, añade bg_enqueue_records y helper is_serializable |
| base_bg/models/bg_job.py | Agrega campos batch_key, next_job_id y cancel_time; implementa encadenamiento de jobs y cancelación en cascada |
| export_bg/models/export_bg_mixin.py | Reemplaza métodos monolíticos de export por sistema de chunks con _export_chunk_bg y _combine_chunks |
| export_bg/static/src/views/list_controller.js | Simplifica llamada al backend usando nuevo método unificado web_export |
| base_bg/views/bg_job_views.xml | Añade campos de batch, nuevo botón para ver jobs del batch, filtros adicionales y remueve readonly de campos |
| base_bg/tests/test_bg_job.py | Añade 12 nuevos tests para batching, linking, cancelación y helpers; elimina 1 test de prioridad |
| base_bg/migrations/18.0.1.0.2/post-migration.py | Script para popular batch_key en jobs existentes con UUIDs únicos |
| base_bg/demo/bg_job_demo.xml | Actualiza datos demo para mostrar batches completos, fallidos y jobs individuales |
| base_bg/manifest.py | Incrementa versión a 18.0.1.0.2 |
| help="Error message from the last failed execution", | ||
| ) | ||
| batch_key = fields.Char( | ||
| required=True, |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El campo 'batch_key' se define como requerido, pero los trabajos existentes en la base de datos no tendrán este valor. Aunque existe un script de migración, debería considerarse usar 'required=False' o agregar un valor por defecto para evitar problemas durante el upgrade si el script falla o se ejecuta en un orden diferente.
| required=True, | |
| required=False, |
| def _combine_chunks(self, export_id, export_format): | ||
| chunks = self.env["ir.attachment"].search([("description", "=", export_id)], order="name") | ||
|
|
||
| if export_format == "csv": | ||
| combined = b"".join( | ||
| base64.b64decode(c.datas) if i == 0 else b"\n".join(base64.b64decode(c.datas).split(b"\n")[1:]) | ||
| for i, c in enumerate(chunks) | ||
| ) | ||
| chunks.unlink() | ||
| return self._save_attachment(combined, ".csv", "text/csv;charset=utf8") | ||
| else: | ||
| buf = io.BytesIO() | ||
| wb = xlsxwriter.Workbook(buf, {"in_memory": True}) | ||
| ws = wb.add_worksheet() | ||
| row_num = 0 | ||
| for chunk in chunks: | ||
| chunk_data = json.loads(base64.b64decode(chunk.datas)) | ||
| if row_num == 0: | ||
| ws.write_row(0, 0, chunk_data["headers"]) | ||
| row_num = 1 | ||
| for row in chunk_data["rows"]: | ||
| ws.write_row(row_num, 0, row) | ||
| row_num += 1 | ||
| wb.close() | ||
| chunks.unlink() | ||
| return self._save_attachment( | ||
| buf.getvalue(), ".xlsx", "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" | ||
| ) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El método '_combine_chunks' no maneja el caso en que no se encuentren chunks. Si 'chunks' está vacío, se producirá un error al intentar crear el attachment con contenido vacío o al acceder a propiedades inexistentes. Se debe agregar validación para verificar que existan chunks antes de procesar.
| end_time=end_time, | ||
| ) | ||
| self.assertEqual(job.duration, 30.0) | ||
|
|
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El test 'test_jobs_are_sorted_by_priority' fue eliminado completamente. Este test verificaba que los jobs se ordenaran correctamente por prioridad, lo cual es funcionalidad importante del sistema. La eliminación de este test reduce la cobertura de pruebas. Debería considerarse mantener o reescribir este test para asegurar que el ordenamiento por prioridad sigue funcionando correctamente.
| cr.execute(""" | ||
| SELECT id FROM bg_job | ||
| WHERE batch_key IS NULL | ||
| """) | ||
| job_ids = [row[0] for row in cr.fetchall()] | ||
| updates = [(str(uuid.uuid4()), job_id) for job_id in job_ids] | ||
| cr.executemany( | ||
| """ | ||
| UPDATE bg_job | ||
| SET batch_key = %s | ||
| WHERE id = %s | ||
| """, | ||
| updates, | ||
| ) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El script de migración no usa procesamiento en lotes. Para instalaciones con muchos registros de bg.job, esto podría causar problemas de memoria o locks largos en la tabla. Se recomienda procesar los registros en chunks de tamaño razonable (ej. 1000 registros por iteración).
| cr.execute(""" | |
| SELECT id FROM bg_job | |
| WHERE batch_key IS NULL | |
| """) | |
| job_ids = [row[0] for row in cr.fetchall()] | |
| updates = [(str(uuid.uuid4()), job_id) for job_id in job_ids] | |
| cr.executemany( | |
| """ | |
| UPDATE bg_job | |
| SET batch_key = %s | |
| WHERE id = %s | |
| """, | |
| updates, | |
| ) | |
| batch_size = 1000 | |
| while True: | |
| cr.execute( | |
| """ | |
| SELECT id FROM bg_job | |
| WHERE batch_key IS NULL | |
| LIMIT %s | |
| """, | |
| (batch_size,), | |
| ) | |
| rows = cr.fetchall() | |
| if not rows: | |
| break | |
| updates = [(str(uuid.uuid4()), row[0]) for row in rows] | |
| cr.executemany( | |
| """ | |
| UPDATE bg_job | |
| SET batch_key = %s | |
| WHERE id = %s | |
| """, | |
| updates, | |
| ) |
| export_data = self.export_data(field_names).get("datas", []) | ||
|
|
||
| if export_format == "csv": | ||
| content = CSVExport().from_data(params["fields"], field_labels, export_data).encode() |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El encoder personalizado 'DateTimeEncoder' solo se usa en el formato JSON intermedio (línea 47), pero no se usa para el formato CSV. Si export_data() retorna objetos datetime, date o time en los datos CSV, estos no se serializarán correctamente. Debería verificarse si CSVExport maneja estos tipos o aplicar el encoding también para CSV.
| content = CSVExport().from_data(params["fields"], field_labels, export_data).encode() | |
| serialized_export_data = [ | |
| [ | |
| (cell.isoformat() if isinstance(cell, (datetime, date, time)) else cell) | |
| for cell in row | |
| ] | |
| for row in export_data | |
| ] | |
| content = CSVExport().from_data(params["fields"], field_labels, serialized_export_data).encode() |
| params = json.loads(data) | ||
| Model = self.env[params["model"]].with_context(**params.get("context", {})) | ||
| records = Model.browse(params["ids"]) if params.get("ids") else Model.search(params.get("domain", [])) | ||
| Model = self.env[params["model"]] | ||
| if params.get("ids"): | ||
| records = Model.browse(params["ids"]) | ||
| else: | ||
| records = Model.search(params.get("domain", [])) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El método 'web_export' no aplica el contexto proporcionado en los parámetros al buscar los registros. La línea 77 crea el modelo sin contexto, mientras que el código anterior usaba 'with_context(**params.get("context", {}))'. Esto podría afectar la aplicación de filtros específicos del contexto como multi-company o filtros temporales.
| ) | ||
|
|
||
| if job and not job._get_next_jobs(): | ||
| return self.env["ir.model"]._combine_chunks(export_id, export_format) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No se maneja el caso de error cuando '_combine_chunks' falla. Si el último trabajo del batch llama a '_combine_chunks' (línea 62) y este método lanza una excepción, el error no será capturado por el sistema de jobs y podría dejar chunks huérfanos sin limpiar. Debería considerarse envolver esta llamada en un try-except o asegurar que los chunks se limpien incluso en caso de error.
| return self.env["ir.model"]._combine_chunks(export_id, export_format) | |
| try: | |
| return self.env["ir.model"]._combine_chunks(export_id, export_format) | |
| except Exception: | |
| # Asegurarse de limpiar los chunks incluso si la combinación falla | |
| attachments = self.env["ir.attachment"].search( | |
| [ | |
| ("res_model", "=", "export.temp"), | |
| ("description", "=", export_id), | |
| ] | |
| ) | |
| attachments.unlink() | |
| raise |
| class DateTimeEncoder(json.JSONEncoder): | ||
| def default(self, obj): | ||
| if isinstance(obj, (datetime, date, time)): | ||
| return obj.isoformat() | ||
| return super().default(obj) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
La clase 'DateTimeEncoder' está definida pero solo se utiliza para el formato JSON intermedio de los chunks XLSX. Sin embargo, cuando se combinan los chunks XLSX (líneas 110-117), los datos JSON se decodifican y escriben directamente en el Excel sin usar el encoder. Si los datos contienen objetos datetime sin serializar en el JSON intermedio, esto funcionará correctamente. Pero el nombre de la clase sugiere un propósito más general que no se está cumpliendo completamente.
| "res_model": "export.temp", | ||
| "res_id": 0, |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El modelo temporal 'export.temp' utilizado como 'res_model' en línea 55 no parece estar definido en el código. Si este modelo no existe en el sistema, la creación del attachment podría fallar. Debería verificarse que el modelo existe o usar un valor diferente (como False o un modelo existente).
| job_vals["args_json"] = list(args) if args else [] | ||
| job_vals["kwargs_json"] = kwargs | ||
| self.env["bg.job"].create(job_vals) | ||
| name = kwargs.get("name", "") |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
El nombre del parámetro 'name' se extrae con kwargs.get pero nunca se elimina del diccionario kwargs con pop(). Esto significa que el parámetro 'name' se pasará también al método del modelo si está presente, lo cual podría causar errores si el método no espera ese parámetro. Debería usarse pop() en lugar de get() como se hace con 'priority' y 'max_retries'.
| name = kwargs.get("name", "") | |
| name = kwargs.pop("name", "") |

…improved record handling