Skip to content

Update PostgresStorageAdapter.js #2087

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

Merged
merged 1 commit into from
Jun 17, 2016
Merged

Update PostgresStorageAdapter.js #2087

merged 1 commit into from
Jun 17, 2016

Conversation

vitaly-t
Copy link
Contributor

@vitaly-t vitaly-t commented Jun 17, 2016

  1. Deleting tables in a transaction, as opposed to just a task.
  2. Added transaction where it was supposed to be. However, it is not enough, the logic may still broken there...

First, do not use .catch before .then. It is dangerous without good understanding how it actually works:

if you do not throw an error in .catch or return a rejected promise explicitly, you will get in the following .then with whatever value that was returned.

Check this out:

        .catch(error => {
          if (error.code === PostgresRelationDoesNotExistError) {
            return this.createClass(className, {fields: {[fieldName]: type}}) // this gets into the following `.then`
          } else if (error.code === PostgresDuplicateColumnError) {

           // this will get into the following `.then` with `undefined` as the value

          } else {
            throw error;
          }
        })

This is why it is always best to have .catch at the bottom, though the logic changes ;)

1. Deleting tables in a transaction, as opposed to just a task.
2. Added transaction where it was supposed to be. However, it is not enough, the logic is still broken there...

First, do not use `.catch` before `.then`. It is dangerous without good understanding how it actually works.

Check this out:
```js
        .catch(error => {
          if (error.code === PostgresRelationDoesNotExistError) {
            return this.createClass(className, {fields: {[fieldName]: type}}) // this gets into the following `.then`
          } else if (error.code === PostgresDuplicateColumnError) {
            // Column already exists, created by other request. Carry on to
            // See if it's the right type.
           
// this will get the following `.then` with `undefined` as the value

          } else {
            throw error;
          }
        })
```
@codecov-io
Copy link

Current coverage is 90.35%

Merging #2087 into master will increase coverage by <.01%

@@             master      #2087   diff @@
==========================================
  Files            93         93          
  Lines          6715       6716     +1   
  Methods        1190       1191     +1   
  Messages          0          0          
  Branches       1411       1411          
==========================================
+ Hits           6067       6068     +1   
  Misses          648        648          
  Partials          0          0          

Powered by Codecov. Last updated by c37a4ea...1e53108

@drew-gross
Copy link
Contributor

In this case the catch before then is intentional, because we want to swallow certain errors, and the next then doesn't need a value anyway. There are other logic errors, which I'm sure will be caught when I get to making the schema tests pass.

I'm not 100% sure we need the transaction in addFieldIfNotExists. We might be able to avoid it if we try to add a field, and if we fail because of an existing field, we check if the field already exists in the _SCHEMA collection, and if it doesn't, and if the column type matches the type the caller wants to add, then update the _SCHEMA collection and resolve, but if it does exist in the _SCHEMA collection, reject with the "already exists" error. We can also still change this API as it's not a public API yet.

Either way, this looks solid for now. Thanks!

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.

3 participants