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: resolve get entity value for date columns that are related #8027

Merged
merged 5 commits into from Aug 8, 2021

Conversation

jordansoltman
Copy link
Contributor

@jordansoltman jordansoltman commented Aug 4, 2021

Closes: #8026

Description of change

Allows for date columns that are part of a relation to have data inserted into them.

Fixes: #8026

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change N/A
  • The new commits follow conventions explained in CONTRIBUTING.md

@imnotjames
Copy link
Contributor

This needs a test to confirm the behavior

@jordansoltman
Copy link
Contributor Author

@imnotjames I added a test. Will it be an issue that my last commit message is a merge?

@imnotjames
Copy link
Contributor

No, it's all gravy. We'll probably squash it.

@imnotjames imnotjames self-requested a review August 4, 2021 23:46
@imnotjames
Copy link
Contributor

We dealt with something similar in #7774 - where only plain objects were treated with this.

Possibly tangentially related kinds of issues.

I'll try to get to actually reviewing this sooner rather than later.

Copy link
Contributor

@imnotjames imnotjames left a comment

Choose a reason for hiding this comment

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

This is fine for now. We probably want to make this more general in the future but I think it'll help your exact issue.

In particular, we should really be checking the metadata to confirm that this property is either a plain object or a registered embedded type & failing otherwise.

@jordansoltman
Copy link
Contributor Author

It does feel like a bandage on a larger issue but I'm very new to TypeORM so I haven't had the time to really understand what the proper fix looks like.

If I stick with TypeORM for future work I'll be happy to circle back and submit a better patch in the future.

@imnotjames imnotjames merged commit 5a3767f into typeorm:master Aug 8, 2021
@jordansoltman jordansoltman deleted the fix-related-date-insert branch August 15, 2021 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants