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

Bug: When we generate our own column names instead of using the columns in the SQL table, the to_dict function does not work #164

Closed
1 of 4 tasks
ysnbyzli opened this issue Apr 26, 2024 · 1 comment · Fixed by #165
Labels
bug Something isn't working

Comments

@ysnbyzli
Copy link
Contributor

ysnbyzli commented Apr 26, 2024

Description

When I want to avoid using the column names found in the SQL table, as in the ExampleModel below, I create custom column names to correspond to them. However, when I try to perform an upsert operation on the table's service, instead of getting the custom column names I provided to the model in the to_dict function, it retrieves the column names from the SQL table. Hence, it does not match the column names in the model.

We've encountered this issue before with the model_from_dict function, and we fixed it by adding the following code. #78

def model_from_dict(model: ModelT, **kwargs: Any) -> ModelT:
    """Return ORM Object from Dictionary."""
    data = {
        column_name: kwargs[column_name]
        for column_name in model.__mapper__.columns.keys()  # noqa: SIM118
        if column_name in kwargs
    }
    return model(**data)  # type: ignore  # noqa: PGH003

If we need to continue from the current issue, we can proceed with the following model.

Model:

class ExampleModel(BigIntAuditBase):
    __tablename__ = "Example_Model"

    field_one: Mapped[str] = mapped_column(
        "FIELDONE", String(10), ForeignKey("ERP_Items.erp_code"), nullable=False
    )
    field_two: Mapped[str] = mapped_column(
        "FIELDTWO", String(10), ForeignKey("ERP_STAR_ITEMS.erp_code"), nullable=False
    )

Current:

    def to_dict(self, exclude: set[str] | None = None) -> dict[str, Any]:
        """Convert model to dictionary.

        Returns:
            dict[str, Any]: A dict representation of the model
        """
        exclude = {"sa_orm_sentinel", "_sentinel"}.union(self._sa_instance_state.unloaded).union(exclude or [])  # type: ignore[attr-defined]
        return {field.name: getattr(self, field.name) for field in self.__table__.columns if field.name not in exclude}

Must be:

def to_dict(self, exclude: set[str] | None = None) -> dict[str, Any]:
        """Convert model to dictionary.

        Returns:
            dict[str, Any]: A dict representation of the model
        """
        exclude = {"sa_orm_sentinel", "_sentinel"}.union(self._sa_instance_state.unloaded).union(exclude or [])  # type: ignore[attr-defined]
        return {field: getattr(self, field) for field in self.__mapper__.columns.keys() if field not in exclude}

URL to code causing the issue

https://github.com/jolt-org/advanced-alchemy/blob/main/advanced_alchemy/base.py#L215

MCVE

# Your MCVE code here

Steps to reproduce

1. Create a model and define column names different from those in the SQL table.
2. Create a service for the model.
3. Use an upsert method within the service.
4. Finally, trigger the place where you used upsert in this service.

Screenshots

"In the format of: ![SCREENSHOT_DESCRIPTION](SCREENSHOT_LINK.png)"

Logs

No response

Jolt Project Version

v0.9.0

Platform

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)

Funding

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
Fund with Polar
@ysnbyzli ysnbyzli added the bug Something isn't working label Apr 26, 2024
ysnbyzli added a commit to ysnbyzli/advanced-alchemy that referenced this issue Apr 26, 2024
cofin added a commit that referenced this issue May 3, 2024
* fix(#164): added mapper column key on to_dict func

* chore: added noqa

---------

Co-authored-by: Cody Fincher <204685+cofin@users.noreply.github.com>
@cofin cofin linked a pull request May 3, 2024 that will close this issue
5 tasks
@cofin
Copy link
Member

cofin commented May 3, 2024

Thanks for the fix here!

@cofin cofin closed this as completed May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants