Master3 #139
Master3 #139
Conversation
…ng\n\n- Add detailed exception logging with full traceback for debugging batch load failures\n- Log complete exception information including type, message, and stack trace\n- Add comprehensive debugging for load response analysis and record creation verification\n- Log detailed information about what we get back from Odoo's model.load() call\n- Add enhanced error detection for load operation responses with proper message handling\n- Improve record creation count logging to show expected vs actual record creation\n- Add detailed warnings when record creation fails or is partial\n- Include sample data logging when no records are created to aid troubleshooting\n- Enhance exception handling to capture full failure information for debugging\n\nThis provides comprehensive debugging information to identify why records aren't being created\nand helps isolate issues in the batch load process, making it easier to troubleshoot\nrecord creation failures and XML ID resolution issues.
…ame 'sanitized_load_lines' is not defined in _execute_load_batch function\n- Correct variable scoping issues in batch load operations\n- Fix undefined variable references in debugging statements\n- Restore proper function implementation with correct variable names\n- Ensure batch load operations use correct variables throughout processing\n\nThis fixes the critical NameError that was preventing batch load operations from\ncompleting and causing record creation failures. The NameError was caused by\nreferencing undefined variables in the debugging and batch load code.\n\nThis resolves the core issue where batch load operations were failing due to\nundefined variable references, preventing successful record creation in Pass 1.
Summary of ChangesHello @bosd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and debuggability of the Odoo data import process within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the batch loading process. It adds sanitization for ID columns to prevent XML ID violations and significantly improves debug logging by adding detailed information about the load response, sanitized data, and full tracebacks for exceptions. The changes are a good step towards more robust error handling and debugging. My review includes a few suggestions to improve code style, readability, and memory efficiency.
| log.debug(f"Full first load_line: {load_lines[0]}") | ||
|
|
||
| # Sanitize the id column values to prevent XML ID constraint violations | ||
| from .lib.internal.tools import to_xmlid |
There was a problem hiding this comment.
Per PEP 8, imports should be at the top of the file, not inside a function. This improves readability and makes dependencies clear. Please move from .lib.internal.tools import to_xmlid to the top-level imports of this module.
| sanitized_load_lines = [] | ||
| for i, line in enumerate(load_lines): | ||
| sanitized_line = list(line) | ||
| if uid_index < len(sanitized_line): | ||
| # Sanitize the source_id (which is in the id column) | ||
| original_id = sanitized_line[uid_index] | ||
| sanitized_id = to_xmlid(original_id) | ||
| sanitized_line[uid_index] = sanitized_id | ||
| if i < 3: # Only log first 3 lines for debugging | ||
| log.debug( | ||
| f"Sanitized ID for line {i}: '{original_id}' -> '{sanitized_id}'" | ||
| ) | ||
| else: | ||
| if i < 3: # Only log first 3 lines for debugging | ||
| log.warning( | ||
| f"Line {i} does not have enough columns for uid_index {uid_index}. " | ||
| f"Line has {len(line)} columns." | ||
| ) | ||
| sanitized_load_lines.append(sanitized_line) |
There was a problem hiding this comment.
This block creates a copy of load_lines to sanitize the ID column. For large batches, this can be memory-inefficient. Since load_lines is not used in its original form after this, you can modify it in-place to save memory. This also makes the code more concise. Assuming load_lines contains mutable lists, you can directly modify each line.
for i, line in enumerate(load_lines):
if uid_index < len(line):
# Sanitize the source_id (which is in the id column)
original_id = line[uid_index]
sanitized_id = to_xmlid(original_id)
line[uid_index] = sanitized_id
if i < 3: # Only log first 3 lines for debugging
log.debug(
f"Sanitized ID for line {i}: '{original_id}' -> '{sanitized_id}'"
)
else:
if i < 3: # Only log first 3 lines for debugging
log.warning(
f"Line {i} does not have enough columns for uid_index {uid_index}. "
f"Line has {len(line)} columns."
)
sanitized_load_lines = load_lines| # Log sample of sanitized data without large base64 content | ||
| log.debug(f"Load header: {load_header}") | ||
| log.debug(f"Load lines count: {len(sanitized_load_lines)}") | ||
| if sanitized_load_lines and len(sanitized_load_lines) > 0: |
There was a problem hiding this comment.
The condition if sanitized_load_lines and len(sanitized_load_lines) > 0: is redundant. In Python, a non-empty list evaluates to True in a boolean context, so if sanitized_load_lines: is sufficient and more idiomatic.
if sanitized_load_lines:| if sanitized_load_lines and len(sanitized_load_lines) > 0: | ||
| # Show first line but truncate large base64 data | ||
| preview_line = [] | ||
| for i, field_value in enumerate(sanitized_load_lines[0][:10] if len(sanitized_load_lines[0]) > 10 else sanitized_load_lines[0]): |
There was a problem hiding this comment.
The conditional expression to slice the first 10 fields is more complex than necessary. Slicing with [:10] already handles cases where the list has fewer than 10 elements gracefully by returning all available elements. You can simplify this.
for i, field_value in enumerate(sanitized_load_lines[0][:10]):| error_str = str(e).lower() | ||
|
|
||
| # Log the full exception for debugging | ||
| import traceback |
There was a problem hiding this comment.
While importing inside a function is sometimes acceptable for modules used only in exceptional paths, traceback is a standard library module and it's generally better practice to place all imports at the top of the file for clarity and consistency. This helps other developers quickly see all module dependencies.
No description provided.