Skip to content

Add support for Oracle DB in recorder#50090

Merged
bdraco merged 7 commits intohome-assistant:devfrom
mciupak:oracle_db
May 23, 2021
Merged

Add support for Oracle DB in recorder#50090
bdraco merged 7 commits intohome-assistant:devfrom
mciupak:oracle_db

Conversation

@mciupak
Copy link
Copy Markdown
Contributor

@mciupak mciupak commented May 4, 2021

Proposed change

This PR adds suport to for Oracle DB to the recorder component. There are 3 commits in this PR:

  • 81c17ed fixes issue with joining two tables when looking into entity history in logbook. Types were not explicitly specified and Oracle DB error was thrown that given types did not match.
  • c0602fe adds Identity for id columns which is mandatory for Oracle DB and removes ondelete action which was "no action" and actally is a "default" action and may be ommited but cannot be specified explicitly for Oracle DB.
    - 0b37293 updates sqlalchemy to 1.4.12 as this version fixes bug with Identity column for MySQL DB

I have tested these changes with SQLite, MySQL, Postrges and of course with Oracle DB. Models were update with Identity column, but I am not sure if it should trigger schema version bump.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @mciupak,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant
Copy link
Copy Markdown

Hey there @dgomes, mind taking a look at this pull request as its been labeled with an integration (sql) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@frenck
Copy link
Copy Markdown
Member

frenck commented May 4, 2021

0b37293 updates sqlalchemy to 1.4.12 as this version fixes bug with Identity column for MySQL DB

This change is not related to this PR and should be split into a separate PR.

@mciupak
Copy link
Copy Markdown
Contributor Author

mciupak commented May 4, 2021

0b37293 updates sqlalchemy to 1.4.12 as this version fixes bug with Identity column for MySQL DB

This change is not related to this PR and should be split into a separate PR.

Alright, but without 0b37293, c0602fe will break MySQL recorder functionality, so this should go together.

@mciupak
Copy link
Copy Markdown
Contributor Author

mciupak commented May 7, 2021

sqlalchemy was already bumped to 1.4.13 by #50138 so I reverted my commit.

@dgomes looks good now?

Comment thread homeassistant/components/recorder/models.py
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 13, 2021

@mciupak Should be good to go once the docs are updated to include Oracle connection strings

https://github.com/home-assistant/home-assistant.io/edit/next/source/_integrations/recorder.markdown

Comment thread homeassistant/components/recorder/models.py Outdated
Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

I took care of fixing the conflict and adding the docs

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 23, 2021

Finished retesting. All looks good

@bdraco bdraco merged commit caad125 into home-assistant:dev May 23, 2021
@github-actions github-actions Bot locked and limited conversation to collaborators May 24, 2021
@mciupak mciupak deleted the oracle_db branch September 2, 2021 06:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

recorder: Unhandled database error while processing event

5 participants