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 #529 2 - "This Session's transaction has been rolled back" #531

Merged
merged 8 commits into from
Jun 2, 2016
Merged

Fix #529 2 - "This Session's transaction has been rolled back" #531

merged 8 commits into from
Jun 2, 2016

Conversation

LAlbertalli
Copy link
Contributor

This pull request fixes the second issue found on #529

Bug:

  • A wrong foreign key has been set on the table columns. It creates a constraint between the column name and the datasource_name in the datasources table.

Fix:

  • Create a migration to change the foreign key to datasource_name in columns -> datasource_names in datasources.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.04% when pulling 29ffe03 on LAlbertalli:fix529-2 into 7d27692 on airbnb:master.

@LAlbertalli
Copy link
Contributor Author

LAlbertalli commented May 27, 2016

Hi,

I noticed that the migrations fails on sqlite. The problem is linked to the fact that SQL Lite does not support alter tables and I have to use a batch alter instead:

    with op.batch_alter_table('columns') as batch_op:
        batch_op.drop_constraint('columns_ibfk_1', type_="foreignkey")
        batch_op.create_foreign_key('columns_ibfk_1', 'datasources', ['datasource_name'], ['datasource_name'])

The problem with this approach is that the sqlite schema doesn't have a name on the foreign key so there's no way I could drop it.

@xrmx
Copy link
Contributor

xrmx commented May 28, 2016

@LAlbertalli since sqlite is the default db please add the batch context manager to keep caravel working out of the box.

@LAlbertalli
Copy link
Contributor Author

@xrmx Please, read my full comment. This kind of migration is not supported in sqlite because the constraint as no name. Otherwise, I would have submitted it immediately. That's why I'm asking for the best approach.

There's a point to be made, though. Foreign keys are not on by default on sqlite, that's probably why none noticed the bug before. Could we modify the init migrations to have the correct foreign key and we conditionally skip the migration for sqlite? I don't like the idea because it will introduce an unpredictable behavior. On the other side, it is the only upgrade path I could foresee that doesn't require an overly complex setup.

I'm open to suggestion.

@xrmx
Copy link
Contributor

xrmx commented May 29, 2016

@LAlbertalli does this help? http://alembic.readthedocs.io/en/latest/batch.html#dropping-unnamed-or-named-foreign-key-constraints

UPDATE: it looks it breaks mysql, see #466 which looks like the same issue as #529

@LAlbertalli
Copy link
Contributor Author

@xrmx Yes, that was the same issue I found trying with the suggestion in the doc you linked.
Another bigger problem is that I'm naming directly the constraint, but that name seems to be autogenerated by MySQL. But I'm not sure the constraint name will be the same on PG that is the third supported database.

I need to do further research, this is why I was asking further suggestion from people more experienced than me in using SQLAlchemy/Alembic.

@x4base
Copy link
Contributor

x4base commented May 31, 2016

@xrmx Seems like it only works when you have created the foreign keys with naming_convention specified. But it's too late now, for those who has been using mysql, the foreign keys have been named according to the default name convention of MySQL. The only solution I have figured out so far is to try on every kind of databases and try to drop the key by trying through each of the default foreign key names.

@LAlbertalli
Copy link
Contributor Author

@xrmx @x4base I see two options moving forward. Modifying the init migration to fix the error. I'm not happy with that but it should solve the issue for new install. People who are already using it, or they don't check foreign keys, or they have already fixed it.

The second option, that I actually like more, is to surround the migration with an exception handling code to skip sqlite and find the correct name for PGSQL.

To be honest, I will go for both actions (change init and use error handling) to cover all the possible issues.

Let me know what do you prefer. It will take me probably few days to provide the patch.

Bye
L.

@x4base
Copy link
Contributor

x4base commented May 31, 2016

I agree with droping the key by the pgsql. But i am concerned about whether we should cover more Sql dialects (e.g. mssql) that sqlalchemy supports or should we just assume that no one is using other dialects?

@LAlbertalli
Copy link
Contributor Author

@x4base I was thinking of sqlite, MySQL, and pg because they are the databases listed in https://github.com/airbnb/caravel/blob/master/caravel/config.py#L35 as an example. But, yes, nothing forbid to use MS SQL or Oracle. If someone has ideas...

@mistercrunch mistercrunch changed the title Fix529 2 Fix #529 2 - "This Session's transaction has been rolled back" Jun 1, 2016
jimexist and others added 6 commits May 31, 2016 23:18
removing duplicated `user_id` def
* Fixing the specific issue

* Added an additional fix for a similar error in #529

Background:
- When an object is modified by SQLAlchemy, it is invalidated so need to be fetched again from the DB
- If there's an exception during a transaction, SQLAlchemy performs a rollback and mark the connection as dirty.

Bug:
- When handling exceptions, the exception handler tries to access the name of the cluster in the main object. Since the name has been invalidated due to a write, SQLAlchemy tries to fetch it on a 'dirty' connection and spits out an error. Solution:
- Fetch the information for handling the exception before starting the process.
…reign keys based on the signature.

It supports also sqlite using batch migrations
@LAlbertalli
Copy link
Contributor Author

The last commit should work for every supported DB.

I've tested it against sqlite and partially tested against MySQL and it worked both upgrade and downgrade.

The "magic" is done by a function that uses the sqlalchemy reflection capability to detect the correct constraint name.

@coveralls
Copy link

coveralls commented Jun 1, 2016

Coverage Status

Coverage remained the same at 82.108% when pulling 7308cf6 on LAlbertalli:fix529-2 into 087c47a on airbnb:master.

@coveralls
Copy link

coveralls commented Jun 1, 2016

Coverage Status

Coverage remained the same at 82.108% when pulling 7308cf6 on LAlbertalli:fix529-2 into 087c47a on airbnb:master.

"fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
}

def find_constraint_name(upgrade = True):
Copy link
Contributor

Choose a reason for hiding this comment

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

On top of this should we probabily generalize this function so can be reused in other migrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree in general.
There's just a question of timing, I don't have much time to dedicate on this bug fix right now and I think this issue is pretty urgent (basically Druid doesn't work on any DB that support Foreign Keys).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, i can do it after this is in :)

@mistercrunch
Copy link
Member

👍

@mistercrunch mistercrunch merged commit fe6628b into apache:master Jun 2, 2016
xrmx added a commit to xrmx/superset that referenced this pull request Jun 3, 2016
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021
…che#531)

Bumps [@storybook/addon-actions](https://github.com/storybookjs/storybook/tree/HEAD/addons/actions) from 5.3.18 to 5.3.19.
- [Release notes](https://github.com/storybookjs/storybook/releases)
- [Changelog](https://github.com/storybookjs/storybook/blob/next/CHANGELOG.md)
- [Commits](https://github.com/storybookjs/storybook/commits/v5.3.19/addons/actions)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021
…che#531)

Bumps [@storybook/addon-actions](https://github.com/storybookjs/storybook/tree/HEAD/addons/actions) from 5.3.18 to 5.3.19.
- [Release notes](https://github.com/storybookjs/storybook/releases)
- [Changelog](https://github.com/storybookjs/storybook/blob/next/CHANGELOG.md)
- [Commits](https://github.com/storybookjs/storybook/commits/v5.3.19/addons/actions)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021
…che#531)

Bumps [@storybook/addon-actions](https://github.com/storybookjs/storybook/tree/HEAD/addons/actions) from 5.3.18 to 5.3.19.
- [Release notes](https://github.com/storybookjs/storybook/releases)
- [Changelog](https://github.com/storybookjs/storybook/blob/next/CHANGELOG.md)
- [Commits](https://github.com/storybookjs/storybook/commits/v5.3.19/addons/actions)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021
…che#531)

Bumps [@storybook/addon-actions](https://github.com/storybookjs/storybook/tree/HEAD/addons/actions) from 5.3.18 to 5.3.19.
- [Release notes](https://github.com/storybookjs/storybook/releases)
- [Changelog](https://github.com/storybookjs/storybook/blob/next/CHANGELOG.md)
- [Commits](https://github.com/storybookjs/storybook/commits/v5.3.19/addons/actions)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants