default values added for new required columns when running autoupdate() #418

karanssj4 · 2020-02-18T17:10:01Z

Fixes #123

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Commit messages are following our guidelines

slnode · 2020-02-18T17:10:05Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

karanssj4 · 2020-02-19T05:22:55Z

@agnes512 npm test fails on jugglerv3/v4, if i just test this particular module, it passes everything.

juggler v3/v4 tests are failing even without making any changes

when running autoupdate(), default values are added for new columns Signed-off-by: Karan Raina <[email protected]>

agnes512 · 2020-02-19T13:14:51Z

@karanssj4 yes v3/v4 sometimes fail randomly :( but don't worry about it in this PR. And thanks for the PR ! I will review it shortly.

agnes512 · 2020-02-19T13:15:05Z

@slnode test please

agnes512 · 2020-02-19T14:43:10Z

@karanssj4 I checked those failures. Actually they are causing by the changes. The tests in this repository are run, so are the tests in Juggler to check the persistence.
If we take a closer look at the one of the falling test cases:
https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/manipulation.test.js#L998

      Player.defineProperty('active', {
        type: Boolean,
        default: false,
      });
      await db.autoupdate('Player');

      // And updateOrCreate an existing record
      const found = await Player.updateOrCreate({id: created.id, name: 'updated'});
      should(found.toObject().active).be.oneOf([
        undefined, // databases supporting `undefined` value
        null, // databases representing `undefined` as `null`
      ]);

it's expecting null or undefined. That's why some v3/v4 tests fail.

I am not sure if it was an intentional decision. But I think active in this case should be false. And this fix makes sense to me. @strongloop/loopback-maintainers could you please confirm?

jannyHou · 2020-02-19T16:02:48Z

@karanssj4 Thank you for the patch! I think the juggler test expects the following behavior:

When a new column with default value gets added to a database, the existing data will have a null value(null or undefined, depends on the db) instead of the default value for that new column.
And new data created omitting that column will have a default value.

So is there a way to follow ^ behaviour when create new columns in postgresql?

lib/migration.js

karanssj4 · 2020-02-19T18:06:59Z

@jannyHou should default values be only added when you add a new required column? Or should they be added for non required columns as well?

I prefer the second one, because If I'm explicitly telling a new column should be added with a default value, then it should have that default value. If I wanted to make new values null, I'd have not added a default value

I just confirmed this is the behavior of pstgresql as well.

ALTER TABLE "tag"
ADD "b6" smallint NULL DEFAULT '11';

This results in a new column b6 and all values are 11 for the existing rows

karanssj4 · 2020-02-19T19:21:08Z

I checked out loopbackio/loopback-datasource-juggler#1704 and
loopbackio/loopback-datasource-juggler#1692

The test case was written so as to NOT return a default value if the database doesn't have it. Here, the database in question does have the default value (that's how Postgres handles it)

@bajtos @jannyHou any thoughts on how should this be handled?

bajtos · 2020-02-20T13:24:05Z

I think I was involved in past discussions about handling default values, I'll try to find them to refresh my memory and then review the proposal and discussion here. I am not sure if I'll find enough time this week, I'll aim to get it done by Tuesday 2020-02-25. (Feel free to ping me if I don't.)

bajtos · 2020-02-21T12:36:50Z

test/postgresql.migration.test.js

@@ -230,3 +260,4 @@ function checkColumns(table, cb) {
    cb(err, cols);
  });
 }
+


Extra EOL at EOF, please remove.

bajtos · 2020-02-21T12:53:03Z

I reviewed the proposed code changes, they look reasonable to me 👍

Regarding the failing test from juggler: this is tricky! As have been correctly wrote above, the test was written with the assumption that when a new optional column with a default value is added to an existing table, existing records will have NULL value in that column. Quoting from loopbackio/loopback-datasource-juggler#1692 (comment):

Let's say we have an existing table with some columns and applications that are already using this table.

Now we add a new column with the following properties:

Existing records are updated to set the new column to NULL (or undefined).

In LB application, we want to set the new colum to a default value when no value was provided by the client. This is achieved by using default metadata in property definition.

When we run the updated application and query the database, the new default value is incorrectly applied on old records.

I think the scenario described above is still valid, we just need to find a way how to improve the test to work with connectors that are setting the newly added column to the default value for existing rows as part of autoupdate.

For example, we can rework the test to define the new property in two steps:

In the first step, the property is defined with no default value and we run autoupdate to update database schema.
In the second step, we add default value to the property definition BUT DO NOT update database schema.

Alternatively, we can look for better ways how to test the scenario we were fixing as part of the pull request which introduced the problematic test. Quoting from loopbackio/loopback-datasource-juggler#1692 (comment) again:

In their project, they are using PersistedModel to access a datasource that's not a database. As a result, the model is describing values that are not always returned back by the connector.

As yet another alternative, we can modify the connector to configure SQL columns with the provided default value only when the property is also required, that may be the easiest way how to move this pull request forward.

I remember that LoopBack treats required and default fields differently than SQL, therefore it may be reasonable to configure default in database only for required properties. Unfortunately, I don't remember details about the differences and the explanation of why LoopBack implements default properties in the current way.

karanssj4 · 2020-02-21T14:52:55Z

@bajtos here's my $0.02 on this. If someone wants a new column with null values, they wouldn't register a default value (at least in SQL).
To support that use case, we do have hooks and remote methods.

If the user doesn't wanna do that..then worst case scenario, they would make a new column without default values, run autoupdate, then add a default value, run autoupdate again

agnes512 · 2020-03-02T15:43:29Z

Similar issue loopbackio/loopback-connector-mongodb#564 (?)

karanssj4 · 2020-03-05T14:31:48Z

@bajtos @jannyHou any pointers on how to proceed finally? should I keep it as is and update juggler tests? should I keep it only for required default values?

If someone wants a new column with null values, they wouldn't register a default value (at least in SQL).
To support that use case, we do have hooks and remote methods.

do you agree with this?

jannyHou

@karanssj4

If someone wants a new column with null values, they wouldn't register a default value (at least in SQL).

I haven't tested with other SQL dbs, but given the native query you post in

ALTER TABLE "tag"
ADD "b6" smallint NULL DEFAULT '11';

I tend to honor postgresql's behavior. The juggler tests passed before because the default column was not built, at least in this connector, so I would go with the fix here and update the juggler test.

Maybe update it to honor a connector's behavior? E.g. postgresql sets default value when new column added while some connector don't (!! might be a wrong assumption, maybe all behave like postgresql).

But I don't know if ^ would violate the fix in PR loopbackio/loopback-datasource-juggler#1704 :p Will respect @bajtos 's opinion on this.

jannyHou · 2020-03-10T03:10:19Z

test/postgresql.migration.test.js

+    await DefaultValueAfterColumnAdd.create({
+      name: 'name1',
+    });
+    await db.defineProperty('DefaultValueAfterColumnAdd', 'createdByAdmin', {


defineProperty is a sync function I think, see its definition

agnes512 · 2020-03-10T12:39:35Z

I can update Juggler's tests if needed :D

bajtos · 2020-03-12T10:47:57Z

But I don't know if ^ would violate the fix in PR loopbackio/loopback-datasource-juggler#1704 :p Will respect @bajtos 's opinion on this.

I am afraid I won't be able to review these changes in the next 2 weeks. @hacksparrow IIRC, you were involved in recent changes related to handling of default values, can you please take a look at this pull request?

stale · 2020-06-26T14:50:19Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-07-11T01:53:28Z

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

karanssj4 · 2020-10-14T17:50:55Z

can we open this again? @bajtos

stale · 2020-12-19T08:57:28Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2021-06-26T03:28:19Z

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

stale · 2021-07-12T19:40:58Z

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

ketansp · 2024-07-13T16:14:06Z

Need to open this again. This is still an issue in 2024.

karanssj4 requested review from agnes512, dhmlau, emonddr and jannyHou as code owners February 18, 2020 17:10

agnes512 self-assigned this Feb 18, 2020

added default values for new columns

c3efea7

when running autoupdate(), default values are added for new columns Signed-off-by: Karan Raina <[email protected]>

karanssj4 force-pushed the master branch from bff6b46 to c3efea7 Compare February 19, 2020 05:29

agnes512 removed their assignment Feb 19, 2020

jannyHou reviewed Feb 19, 2020

View reviewed changes

lib/migration.js Show resolved Hide resolved

bajtos self-requested a review February 20, 2020 13:24

bajtos reviewed Feb 21, 2020

View reviewed changes

agnes512 mentioned this pull request Mar 2, 2020

Monthly Milestone - March 2020 👾⏳🐨 loopbackio/loopback-next#4767

Closed

23 tasks

jannyHou reviewed Mar 10, 2020

View reviewed changes

dhmlau mentioned this pull request Apr 1, 2020

Monthly Milestone - April 2020 💪😷🐣 loopbackio/loopback-next#5010

Closed

37 tasks

bajtos added the community-contribution Patches contributed by community label Apr 27, 2020

jannyHou mentioned this pull request Apr 30, 2020

Monthly Milestone - May 2020 ☀️ 🌎 🍀 loopbackio/loopback-next#5301

Closed

39 tasks

stale bot added the stale label Jun 26, 2020

stale bot closed this Jul 11, 2020

agnes512 reopened this Oct 14, 2020

stale bot removed the stale label Oct 14, 2020

stale bot added the stale label Dec 19, 2020

stale bot closed this Jul 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

default values added for new required columns when running autoupdate() #418

default values added for new required columns when running autoupdate() #418

karanssj4 commented Feb 18, 2020 •

edited

Loading

slnode commented Feb 18, 2020

karanssj4 commented Feb 19, 2020 •

edited

Loading

agnes512 commented Feb 19, 2020

agnes512 commented Feb 19, 2020

agnes512 commented Feb 19, 2020 •

edited

Loading

jannyHou commented Feb 19, 2020

karanssj4 commented Feb 19, 2020 •

edited

Loading

karanssj4 commented Feb 19, 2020

bajtos commented Feb 20, 2020

bajtos Feb 21, 2020

bajtos commented Feb 21, 2020

karanssj4 commented Feb 21, 2020

agnes512 commented Mar 2, 2020

karanssj4 commented Mar 5, 2020

jannyHou left a comment

jannyHou Mar 10, 2020

agnes512 commented Mar 10, 2020

bajtos commented Mar 12, 2020

stale bot commented Jun 26, 2020

stale bot commented Jul 11, 2020

karanssj4 commented Oct 14, 2020

stale bot commented Dec 19, 2020

stale bot commented Jun 26, 2021

stale bot commented Jul 12, 2021

ketansp commented Jul 13, 2024

default values added for new required columns when running autoupdate() #418

default values added for new required columns when running autoupdate() #418

Conversation

karanssj4 commented Feb 18, 2020 • edited Loading

Checklist

slnode commented Feb 18, 2020

karanssj4 commented Feb 19, 2020 • edited Loading

agnes512 commented Feb 19, 2020

agnes512 commented Feb 19, 2020

agnes512 commented Feb 19, 2020 • edited Loading

jannyHou commented Feb 19, 2020

karanssj4 commented Feb 19, 2020 • edited Loading

karanssj4 commented Feb 19, 2020

bajtos commented Feb 20, 2020

bajtos Feb 21, 2020

Choose a reason for hiding this comment

bajtos commented Feb 21, 2020

karanssj4 commented Feb 21, 2020

agnes512 commented Mar 2, 2020

karanssj4 commented Mar 5, 2020

jannyHou left a comment

Choose a reason for hiding this comment

jannyHou Mar 10, 2020

Choose a reason for hiding this comment

agnes512 commented Mar 10, 2020

bajtos commented Mar 12, 2020

stale bot commented Jun 26, 2020

stale bot commented Jul 11, 2020

karanssj4 commented Oct 14, 2020

stale bot commented Dec 19, 2020

stale bot commented Jun 26, 2021

stale bot commented Jul 12, 2021

ketansp commented Jul 13, 2024

karanssj4 commented Feb 18, 2020 •

edited

Loading

karanssj4 commented Feb 19, 2020 •

edited

Loading

agnes512 commented Feb 19, 2020 •

edited

Loading

karanssj4 commented Feb 19, 2020 •

edited

Loading