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

Create an Oracle dialect #2891

Closed
wants to merge 88 commits into from
Closed

Conversation

jimlambrt
Copy link
Contributor

@jimlambrt jimlambrt commented Feb 18, 2020

Make sure these boxes checked before submitting your pull request.

  • Do only one thing
  • No API-breaking changes
  • New code/logic commented & tested

For significant changes like big bug fixes, new features, please open an issue to make an agreement on an implementation design/plan first before starting it.

What did this pull request do?

Implement support for oracle and an oracle dialect: oci8.

Why add an oracle dialect directly to gorm, instead of just writing one in a separate repo?
There are several parts of scope.go that include incompatible SQL, and gorm currently doesn't support overriding those behaviors, so a few judicious changes were needed, which were all prefaced with a call to scope.IsOra() to isolate them from the rest of the implementation

This PR includes:

  • Define an OraCommon type which implements all the common behaviors for Oracle across drivers. Please see dialect_oracommon.go where you'll find documentation for many of the design/implementation choices and why they were made.
  • Define an OraDialect interface so other oracle drivers can override behavior around "last insert id" and make it easy to assert that a dialect implements oracle
  • Define a new dialect "oci8" that uses the driver github.com/mattn/go-oci8 I've also included a README that documents how-to install the driver and its dependencies along with Oracle for testing.
  • All existing unit tests pass. This required cleaning up some trailing semicolons in existing tests and adding a few oracle specific cases to handle how oracle deals with identifiers (quoting and upper casing). These changes are all prefaced by a call to the isOra() func.

Thank you for you careful consideration of this PR. -Jim

@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

Merging #2891 into master will increase coverage by 0.49%.
The diff coverage is 80.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2891      +/-   ##
==========================================
+ Coverage   79.01%   79.50%   +0.49%     
==========================================
  Files          24       25       +1     
  Lines        3540     3737     +197     
==========================================
+ Hits         2797     2971     +174     
- Misses        642      659      +17     
- Partials      101      107       +6     
Impacted Files Coverage Δ
scope.go 87.06% <70.00%> (+0.02%) ⬆️
dialect_oracommon.go 82.24% <82.24%> (ø)
callback_create.go 86.66% <100.00%> (+0.34%) ⬆️
callback_query_preload.go 87.50% <0.00%> (-1.25%) ⬇️
association.go 93.27% <0.00%> (+0.89%) ⬆️
main.go 82.02% <0.00%> (+0.92%) ⬆️
callback_query.go 86.20% <0.00%> (+3.44%) ⬆️
utils.go 89.56% <0.00%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9500e1...d9500e1. Read the comment docs.

@jimlambrt
Copy link
Contributor Author

jimlambrt commented Feb 20, 2020

Getting close on the wercker builds. I had to run the code coverage test twice and merge it with github.com/wadey/gocovmerge since the default just runs the sqlite tests and reports 0% coverage on my new common ora dialect. At the moment, the mariadb container is failing to start, which isn't something I changed... hum. Re-running local tests, after deleting all my local docker containers/vols to see if I can reproduce the issue.

BTW, running oracle in a container is a dumpster fire no doubt. I used the official docker repo for building it: https://github.com/oracle/docker-images. Wow, it's a big container and takes forever to start up.

lingceng and others added 2 commits March 5, 2020 11:55
Panic will rollback case should use "transaction-3".
Fix TestTransactionWithBlock panic case
@nlamot
Copy link

nlamot commented Apr 28, 2020

Hi @jimlambrt, this looks great! Is there any news on when this will be merged to master?
We would like to use GORM as well to connect to an Oracle DB.

@jimlambrt
Copy link
Contributor Author

@nlamot At this point, I think we're waiting for v2 before anything gets merged... and I'm not sure what that means for this new dialect.

@jimlambrt
Copy link
Contributor Author

Well, looks like the latest mysql image (3 days ago) broke the automated wercker tests for mysql

@@ -792,7 +792,12 @@ func TestRelated(t *testing.T) {

var creditcard CreditCard
var user3 User
DB.First(&creditcard, "number = ?", "1234567890")
numberIs := "number = ?"
if isOra(DB) {

Choose a reason for hiding this comment

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

I think that it is better to remove specific database judging. It should be implemented separately (I know oracle is very different).

@jinzhu
Copy link
Member

jinzhu commented Jun 10, 2020

The PR is closed due to it is developed based on V1, I’d really appreciate it if someone would like to develop it based on V2.

@jinzhu
Copy link
Member

jinzhu commented Jun 10, 2020

@jimlambrt let me know if you would like to create a repo on https://github.com/go-gorm, thank you.

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.

None yet

7 participants