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

When new required fields with a default value are added autoupdate shouldn't fail #123

Closed
faridnsh opened this issue Jan 12, 2016 · 18 comments
Labels

Comments

@faridnsh
Copy link

faridnsh commented Jan 12, 2016

Steps to produce:

1 - Create a model and run autoupdate/automigrate
2 - Add a new required property with a default value to the model for example:

    "newProperty": {
      "type": "Number",
      "required": true,
      "default": 0
    }

3 - Run autoupdate again

What's expected: New column should be added with the default value.

What actually happens: Autoupdate fails here and throws this:

error: column "newProperty" contains null values

It happens because it tries to add column with NOT NULL constraint, but this is only possible, if we give a default value, but loopback-connector-postgresql ignores the given default value in the model configuration.

I would like to contribute this myself, but I'm not sure if I'm getting the test configuration to work. I'm getting error: password authentication failed for user "user" for the first test, but the credentials in the configuration are correct and also a lot of other tests are failing.

@Elindorath
Copy link

@raymondfeng @superkhau @bajtos : I would like to address this issue, but I'm unable to run the test right now. I saw on the Readme that I need to contact you for instructions so I do.

@raymondfeng
Copy link
Contributor

@Elindorath You have two ways to configure your postgres connection.

  1. Set up env vars (https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/init.js#L10)
export CI=true
export TEST_POSTGRESQL_HOST=localhost
export TEST_POSTGRESQL_PORT=5432
export TEST_POSTGRESQL_USER=myUser
export TEST_POSTGRESQL_PASSWORD=myPassword

Or drop .loopbackrc file into your home dir:

{
    "test": {
        "postgresql": {
            "host": "localhost",
            "port": 5432,
            "database": "test",
            "username": "test",
            "password": "testpass"
        }
    }
}

Please also use https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/tables.sql to populate your test db.

@Elindorath
Copy link

Thanks @raymondfeng. I'll try to give you an update this week-end.

@zihehuang
Copy link

zihehuang commented Jul 15, 2016

@Elindorath Any update on this issue? It seems that the connector is trying to add the column and set the NOT NULL attribute at the same time.

The way I worked around it was to drop the "required": true field and add the column to the database first. I then manually set the value of the column in Postgres. However, this is no different from doing everything manually and defeats the purpose of using a connector.

MichaelMarner pushed a commit to NextFaze/loopback-connector-postgresql that referenced this issue Jul 27, 2016
If a model property has a default value, set that as the database column's default value when adding the column in an auto update.

This ensures that existing database rows will have a value for the new column, and prevents the autoupdate failing due to null values.

Fixes loopbackio#123
MichaelMarner pushed a commit to NextFaze/loopback-connector-postgresql that referenced this issue Jul 27, 2016
If a model property has a default value, set that as the database column's default value when adding the column in an auto update.

This ensures that existing database rows will have a value for the new column, and prevents the autoupdate failing due to null values.

Fixes loopbackio#123
STRML added a commit to STRML/loopback-connector-postgresql that referenced this issue Sep 12, 2016
This is a mergeable update to loopbackio#153 that also respects dbDefault.
STRML added a commit to STRML/loopback-connector-postgresql that referenced this issue Sep 12, 2016
This is a mergeable update to loopbackio#153 that also respects dbDefault.
STRML added a commit to STRML/loopback-connector-postgresql that referenced this issue Sep 12, 2016
This is a mergeable update to loopbackio#153 that also respects dbDefault.
STRML added a commit to STRML/loopback-connector-postgresql that referenced this issue Sep 12, 2016
This is a mergeable update to loopbackio#153 that also respects dbDefault.
STRML added a commit to STRML/loopback-connector-postgresql that referenced this issue Oct 4, 2016
This is a mergeable update to loopbackio#153 that also respects dbDefault.
superkhau pushed a commit that referenced this issue Oct 4, 2016
This is a mergeable update to #153 that also respects dbDefault.
superkhau pushed a commit that referenced this issue Oct 4, 2016
Fixes #123. This is a mergeable update to #153 that also respects
dbDefault.

Backport of #167
superkhau added a commit that referenced this issue Oct 4, 2016
Fixes #123. This is a mergeable update to #153 that also respects
dbDefault.

Backport of #167
0candy added a commit that referenced this issue Oct 14, 2016
 * Add connectorCapabilities global object (#179) (Nicholas Duffy)
 * Accept PGDATABASE env var in test/init.js (#178) (Simon Ho)
 * Remove unused prefix from test env vars (#176) (Simon Ho)
 * Fix #123: Set default value during autoupdate. (#167) (Samuel Reed)
 * Update translation files - round#2 (#170) (Candy)
 * Add translated files (gunjpan)
 * Update deps to loopback 3.0.0 RC (Miroslav Bajtoš)
 * Use juggler@3 for running tests (Candy)
 * Add eslint infrastructure (Loay)
 * Revert "Add eslint infrastructure" (Loay)
 * Fix CI Failure (Loay)
 * test: accept more env vars on CI (Ryan Graham)
 * test: use 'emptytest' database as default (Ryan Graham)
 * test: seed DB with test schema before running (Ryan Graham)
 * test: separate dbconfig from datasource (Ryan Graham)
 * test: replace tables.sql with full schema dump (Ryan Graham)
 * Refactor (jannyHou)
 * Upgrade version (jannyHou)
 * Globalize discover.js (jannyHou)
 * Update URLs in CONTRIBUTING.md (#150) (Ryan Graham)
@kutsyk
Copy link

kutsyk commented Aug 2, 2018

Is this fixed?

@bvanleeuwen1995
Copy link

To me this seems to be still a bug, either defaultFn or default is not honerred during migrations and it will fail.

@ptmccarthy
Copy link

I'm still running into this issue today

@bartshappee
Copy link

I ran into this today as well, the manual workaround is dangerous in the long term

JumpLao added a commit to JumpLao/loopback-connector-postgresql that referenced this issue Dec 23, 2018
@JumpLao JumpLao mentioned this issue Dec 23, 2018
1 task
@devpascoe
Copy link

devpascoe commented Mar 1, 2019

same, looks like code isn't merged to master yet.
until then .... ;) npm install --save https://github.com/JumpLao/loopback-connector-postgresql.git
but .... looks like columnDbDefault isn't looking for defaultFn only default. Its not using the same default value checking as found in lookback-datasource-juggler lib/model.js _initProperties.

@bitmage
Copy link

bitmage commented Apr 23, 2019

I'm running into this today, and it's breaking our production deploy.

@bitmage
Copy link

bitmage commented Apr 24, 2019

Ended up running manual ALTER statements. 👎

@kgrvamsi
Copy link

@raymondfeng @bajtos need your help here how to resolve this issue?is this fix part of the connector or we need to do this patch on the existing library to bypass the main issue?

@piuspbd
Copy link

piuspbd commented Sep 5, 2019

Still running into the issue today.

@sraddhan
Copy link

For those who end up here and are wondering about the status of the issue, there's similar issue reported which is open as of this moment. here is the link.

@stale
Copy link

stale bot commented Apr 18, 2020

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 stale bot added the stale label Apr 18, 2020
@agnes512 agnes512 removed their assignment Apr 30, 2020
@stale stale bot removed the stale label Apr 30, 2020
@stale
Copy link

stale bot commented Jul 3, 2020

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 stale bot added the stale label Jul 3, 2020
@stale
Copy link

stale bot commented Jul 18, 2020

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 stale bot closed this as completed Jul 18, 2020
@ketansp
Copy link

ketansp commented Jul 13, 2024

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

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