-
Notifications
You must be signed in to change notification settings - Fork 5
fix: preserve _meta during ACP serialization #20
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
Conversation
Signed-off-by: Chojan Shang <chojan.shang@vesoft.com>
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
This PR adds support for field aliases in Pydantic models, specifically to map the field_meta attribute to the _meta JSON field for RPC serialization. The change enables proper serialization/deserialization of metadata fields using the JSON-RPC convention.
Key changes:
- Added
populate_by_name=Trueconfiguration to allow models to accept both field names and their aliases during instantiation - Updated serialization to use
by_alias=Trueto output JSON with aliased field names (_metainstead offield_meta) - Modified schema generation to automatically inject custom BaseModel configuration
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/acp/schema.py | Adds custom BaseModel class with populate_by_name=True configuration to support field aliases |
| src/acp/utils.py | Updates serialize_params to use by_alias=True for proper JSON field name mapping |
| src/acp/connection.py | Updates response serialization to use aliases and additional JSON-safe options |
| scripts/gen_schema.py | Adds schema generation logic to automatically inject custom BaseModel configuration |
| tests/test_utils.py | Adds test coverage for alias serialization behavior |
| examples/echo_agent.py | Demonstrates usage of field_meta attribute with the new alias support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not line.startswith("from pydantic import "): | ||
| continue | ||
| imports = [part.strip() for part in line[len("from pydantic import ") :].split(",")] | ||
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | ||
| has_config = any(part == "ConfigDict" for part in imports) | ||
| new_imports = [] | ||
| for part in imports: | ||
| if part == "BaseModel": | ||
| new_imports.append("BaseModel as _BaseModel") | ||
| has_alias = True | ||
| else: | ||
| new_imports.append(part) | ||
| if not has_alias: | ||
| new_imports.append("BaseModel as _BaseModel") | ||
| if not has_config: | ||
| new_imports.append("ConfigDict") | ||
| lines[idx] = "from pydantic import " + ", ".join(new_imports) | ||
| insert_idx = idx + 1 | ||
| lines.insert(insert_idx, "") | ||
| lines.insert(insert_idx + 1, "class BaseModel(_BaseModel):") | ||
| lines.insert(insert_idx + 2, " model_config = ConfigDict(populate_by_name=True)") | ||
| lines.insert(insert_idx + 3, "") | ||
| break |
Copilot
AI
Nov 2, 2025
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.
The loop continues after finding the first from pydantic import line, but it should break after processing. This will cause issues if there are multiple pydantic import lines, as subsequent iterations will try to insert the BaseModel class definition multiple times at different positions.
| if not line.startswith("from pydantic import "): | |
| continue | |
| imports = [part.strip() for part in line[len("from pydantic import ") :].split(",")] | |
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | |
| has_config = any(part == "ConfigDict" for part in imports) | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| has_alias = True | |
| else: | |
| new_imports.append(part) | |
| if not has_alias: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if not has_config: | |
| new_imports.append("ConfigDict") | |
| lines[idx] = "from pydantic import " + ", ".join(new_imports) | |
| insert_idx = idx + 1 | |
| lines.insert(insert_idx, "") | |
| lines.insert(insert_idx + 1, "class BaseModel(_BaseModel):") | |
| lines.insert(insert_idx + 2, " model_config = ConfigDict(populate_by_name=True)") | |
| lines.insert(insert_idx + 3, "") | |
| break | |
| if line.startswith("from pydantic import "): | |
| imports = [part.strip() for part in line[len("from pydantic import ") :].split(",")] | |
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | |
| has_config = any(part == "ConfigDict" for part in imports) | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| has_alias = True | |
| else: | |
| new_imports.append(part) | |
| if not has_alias: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if not has_config: | |
| new_imports.append("ConfigDict") | |
| lines[idx] = "from pydantic import " + ", ".join(new_imports) | |
| insert_idx = idx + 1 | |
| lines.insert(insert_idx, "") | |
| lines.insert(insert_idx + 1, "class BaseModel(_BaseModel):") | |
| lines.insert(insert_idx + 2, " model_config = ConfigDict(populate_by_name=True)") | |
| lines.insert(insert_idx + 3, "") | |
| break |
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | ||
| has_config = any(part == "ConfigDict" for part in imports) | ||
| new_imports = [] | ||
| for part in imports: | ||
| if part == "BaseModel": | ||
| new_imports.append("BaseModel as _BaseModel") | ||
| has_alias = True | ||
| else: | ||
| new_imports.append(part) | ||
| if not has_alias: | ||
| new_imports.append("BaseModel as _BaseModel") | ||
| if not has_config: |
Copilot
AI
Nov 2, 2025
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.
The conditions if not has_alias and if not has_config will always be False when the loop at line 235-240 already found BaseModel and converted it to the alias. Since line 232-233 check for existing alias/config, and lines 236-238 ensure the alias is added when BaseModel is found, these additional checks are redundant. The logic should either rely on the existing checks (lines 232-233) or simplify this section.
| has_alias = any(part == "BaseModel as _BaseModel" for part in imports) | |
| has_config = any(part == "ConfigDict" for part in imports) | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| has_alias = True | |
| else: | |
| new_imports.append(part) | |
| if not has_alias: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if not has_config: | |
| new_imports = [] | |
| for part in imports: | |
| if part == "BaseModel": | |
| new_imports.append("BaseModel as _BaseModel") | |
| else: | |
| new_imports.append(part) | |
| # Ensure "BaseModel as _BaseModel" and "ConfigDict" are present | |
| if "BaseModel as _BaseModel" not in new_imports: | |
| new_imports.append("BaseModel as _BaseModel") | |
| if "ConfigDict" not in new_imports: |
Fix: #18