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

Bug/missing AllowRoot panics no context methods #227

Merged
merged 2 commits into from
Aug 29, 2023
Merged

Bug/missing AllowRoot panics no context methods #227

merged 2 commits into from
Aug 29, 2023

Conversation

nilsolofsson
Copy link
Contributor

If you setup otelsql without passing otelsql.AllowRoot() any calls to a "No Context" variant of the query methods will cause a panic in the runtime.

This PR solves this by making sure that methodTracer isn't null before applying the queryTrace function to the middlewares.

@nhatthm
Copy link
Owner

nhatthm commented Aug 29, 2023

Hi, I don't get the problem. Could you please elaborate?

any calls to a "No Context" variant of the query

@nilsolofsson
Copy link
Contributor Author

Yeah sorry, should have a personal proof reader 😄
The issue is specifically (atleast from my investigation) happening for prepared statements. But if you create a prepared statement and call any "non-context" query method there will be a nil-pointer panic, due to the current makeQueryerContextMiddlewares adding a middleware that references a nil interface.

To clarify here's an example that produces the panic without this PR.

	drivername, err := otelsql.Register("postgres",
		// uncomment to allow otelsql to create root spans
		// otelsql.AllowRoot(),
		otelsql.TraceQueryWithoutArgs(),
		otelsql.TraceRowsAffected(),
		otelsql.TraceLastInsertID(),
	)
	if err != nil {
		panic(err)
	}

	sqlDB, err := sql.Open(drivername, dataSourceName)
	if err != nil {
		return err
	}
	sqlxDB := sqlx.NewDb(sqlDB, "postgres")

	stmt, err := sqlxDB.Prepare("SELECT 1")
	if err != nil {
		return err
	}

	_, err = stmt.Query() // <--- This will result in a panic due to nil pointers
	if err != nil {
		return err	
	}

@nhatthm
Copy link
Owner

nhatthm commented Aug 29, 2023

Cool! Thanks for the example ❤️

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #227 (0c5693b) into master (3e506ca) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #227   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         1217      1219    +2     
=========================================
+ Hits          1217      1219    +2     
Flag Coverage Δ
unittests-Linux-X64 100.00% <100.00%> (ø)
unittests-Windows-X64 100.00% <100.00%> (ø)
unittests-macOS-X64 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
query.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nhatthm nhatthm merged commit df931df into nhatthm:master Aug 29, 2023
78 checks passed
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.

2 participants