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

adds an option to control whether columns are inserted by default #4035

Merged
merged 7 commits into from
May 4, 2019

Conversation

paulfitz
Copy link
Contributor

This adds an insert option, analogous to the existing select and readonly options, for use in the Column decorator. Just like a column specified with @Column({select: false}) will not be selected by default in queries, and a column with@Column({readonly: true;}) will not be UPDATEd, a column specified with @Column({insert: false}) will not be inserted by default in INSERTs.

Adds a test to verify that an entity can be created even if an alleged column is not present, if that column is marked insert: false.

I believe this fixes #85, and is relevant to (though not the same as) the work in #2150.

@pleerock
Copy link
Member

Shall we deprecate readonly and introduce a update with the same functionality to prevent naming confusion?

@paulfitz
Copy link
Contributor Author

paulfitz commented Apr 29, 2019

Shall we deprecate readonly and introduce a update with the same functionality to prevent naming confusion?

I'd be happy to include that in this PR, given that you're in favor of it. To deprecate something in typeorm, is it sufficient to add a @deprecated tag in the right place, and a note to the ChangeLog?

@pleerock
Copy link
Member

pleerock commented May 1, 2019

I'd be happy to include that in this PR, given that you're in favor of it. To deprecate something in typeorm, is it sufficient to add a @deprecated tag in the right place, and a note to the ChangeLog?

correct. Feel free to do that if you are able to!

@paulfitz
Copy link
Contributor Author

paulfitz commented May 2, 2019

@pleerock I was tracing the current readonly flag so I could add update instead (slightly non-trivial since the logic is inverted). I trace the effect of the flag to ColumnMetaData.isReadonly, but then I can't find a place where that flag is read. I tried working back from the unit test in test/functional/columns/readonly/columns-readonly.ts but that test looks strange, it seems to expect all columns to change including the one marked readonly. I've found an existing discussion of this at #2749 (comment). In summary, the existing implementation of readonly looks broken, so I'm scared about touching it in this PR since it'll be hard to validate the changes.

@pleerock
Copy link
Member

pleerock commented May 2, 2019

@paulfitz maybe you just deprecate it and introduce new update: boolean parameter that would do what readonly supposed to do? I won't break any exist functionality, right?

This adds an `insert` option, analogous to the existing `select`
and `readonly` options, for use in the `Column` decorator.  Just
like a column specified with `@Column({select: false})` will not
be selected by default in queries, and a column with
`@Column({readonly: true;})` will not be `UPDATE`d,
a column specified with `@Column({insert: false})`
will not be inserted by default in `INSERT`s.

I believe this fixes typeorm#85, and is relevant to (though not the same as)
the work in typeorm#2150.
@paulfitz
Copy link
Contributor Author

paulfitz commented May 2, 2019

@pleerock I took a shot at implementing the update option, deprecating readonly, leaving the existing readonly test untouched and adding a new test for update+insert options. Let me know how it feels.

Edit: oops, I accidentally fixed the readonly option and needed to update its test.

CHANGELOG.md Outdated
@@ -17,6 +17,7 @@ feel free to ask us and community.
* sqlite date hydration is susceptible to corruption ([#3949](https://github.com/typeorm/typeorm/issues/3949))
* fixed mongodb uniques, support 3 ways to define uniques ([#3986](https://github.com/typeorm/typeorm/pull/3986))
* fixed mongodb TTL index ([#4044](https://github.com/typeorm/typeorm/pull/4044))
* deprecate column `readonly` option in favor of `update` and `insert` options ([#4035](https://github.com/typeorm/typeorm/pull/4035))
Copy link
Member

Choose a reason for hiding this comment

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

since 0.2.17 has been released this should be moved into new section. damn hate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only thing I can think of which could minimize this is to use some kind of changelog generator, but to make it work we would have to write commit messages in specific format.
We wouldn't have to fore contributors to do so - changing messages of merge commits should be enough. Which means a bit more work when merging PRs(decide if change should be seen in changelog, and what entry should be added there).

Copy link
Member

Choose a reason for hiding this comment

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

I guess it will be our job - change title before merging. Thats okay I think... we need to find some good changelog generator solution then.

@pleerock pleerock merged commit 7d7fd9f into typeorm:master May 4, 2019
@pleerock
Copy link
Member

pleerock commented May 4, 2019

Thank you very much for this amazing PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Column option insert: false, update: false
3 participants