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.

Sign up for GitHub

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

Jump to bottom

stmt: export WrappedParentStmt to allow Stmt wrapping in interceptors #16

Closed
wants to merge 4 commits into from
Closed

stmt: export WrappedParentStmt to allow Stmt wrapping in interceptors #16

wants to merge 4 commits into from

Conversation

fho
Copy link
Contributor

@fho fho commented Sep 6, 2021 •
edited
Loading 

    stmt: export WrappedParentStmt to allow Stmt wrapping in interceptors

    In some scenarios it is required to annotate Stmt, Rows and Tx objects
    with additional data in an interceptor.
    Usecases can be:
    - relating a PrepareContext() call with operations on the Stmt in
      traces,
    - forwarding the query string passed to PrepareContext() to methods of
      the Stmt

    For Rows and Tx objects it is possible in an interceptor by creating a
    struct that wraps the original objects and has additional user-defined
    fields.
    The user-wrapped object is passed to the RowsNext, RowsClose, TxCommit
    and TxRollback interceptor methods. In those a type-conversion can be
    done and the user-defined data accessed.

    For Stmt objects this worked only for the StmtClose call.
    When the StmtQueryContext StmtExecContext methods of the interceptor
    were called, the driver.Stmt that was passed as parameter was wrapped
    into the private wrappedStmt struct by sqlmw.
    Because it was private, an interceptor implementation could not convert
    the Stmt parameter type and access it's own wrapped Stmt.

    This commit allows also to wrap Stmts in an interceptor and annotate it
    with additional data.
    This is done by making WrappedParentStmt public and changing the
    interceptor interface to  pass stmt of this type.
    Additionally:
    - wrappedParentStmt is renamed to Stmt,
    - a Stmt.Parent() helper method is added,
    - a testcase and example is added

This resolves #10

fho added 4 commits September 6, 2021 15:30
@fho
stmt: export WrappedParentStmt to allow Stmt wrapping in interceptors
d5974f8 
In some scenarios it is required to annotate Stmt, Rows and Tx objects
with additional data in an interceptor.
Usecases can be:
- relating a PrepareContext() call with operations on the Stmt in
  traces,
- forwarding the query string passed to PrepareContext() to methods of
  the Stmt

For Rows and Tx objects it is possible in an interceptor by creating a
struct that wraps the original objects and has additional user-defined
fields.
The user-wrapped object is passed to the RowsNext, RowsClose, TxCommit
and TxRollback interceptor methods. In those a type-conversion can be
done and the user-defined data accessed.

For Stmt objects this worked only for the StmtClose call.
When the StmtQueryContext StmtExecContext methods of the interceptor
were called, the driver.Stmt that was passed as parameter was wrapped
into the private wrappedStmt struct by sqlmw.
Because it was private, an interceptor implementation could not convert
the Stmt parameter type and access it's own wrapped Stmt.

This commit allows also to wrap Stmts in an interceptor and annotate it
with additional data.
This is done by making WrappedParentStmt public and changing the
interceptor interface to  pass stmt of this type.
Additionally:
- wrappedParentStmt is renamed to Stmt,
- a Stmt.Parent() helper method is added,
- a testcase and example is added
@fho
readme: add note about project status
03cca04
@fho
readme: add section documenting breaking API changes
730b2cd
@fho
readme: link stmt data forwarding example
db08673
@fho fho force-pushed the export_wrapped_stmt branch from 7d6add2 to db08673 Compare September 6, 2021 13:33
@fho fho changed the title Export wrapped stmt stmt: export WrappedParentStmt to allow Stmt wrapping in interceptors Sep 6, 2021
@fho fho marked this pull request as ready for review September 6, 2021 13:36
@fho fho mentioned this pull request Sep 6, 2021
Closed
@tcolgate
Copy link
Contributor

Are #15 and #16 going to be merged soon? (currently working on some DB driver instrumentation and would rather add the statement wrapping sooner rather than later).

@fho
Copy link
Contributor Author

fho commented Nov 23, 2021

@tcolgate fyi, I gave up on getting my PRs merged here and created a fork: https://github.com/simplesurance/sqlmw

@fho fho mentioned this pull request Dec 3, 2021
Merged
@inconshreveable
Copy link
Contributor

@fho i'd like to unblock you here and get these merged in but I don't have the time to review. @tcolgate could you take a look at both PRs and review them so that we can get them merged in?

@inconshreveable
Copy link
Contributor

@tcolgate any chance you'd be able to help review this and #15 ?

@tcolgate
Copy link
Contributor

tcolgate commented Jan 7, 2022 •
edited
Loading

I don't have a tonne of time. My first thought though is that, since we employed the Unwrap approach in my Rows PR, would it make sense for this PR to follow that lead?
It might be possible to do it without the API breakage (though at the cost of requiring an extra type switch (thought, practically, the users has to type switch the result of Parent anyway).
Another reason is that it may allow middleware to compose more easily, if there are multiple middleware wrapping a Stmt.

@fho
Copy link
Contributor Author

fho commented Jan 7, 2022

Yes, would be best to have a consistent approach.
It's not clear to me how we could use the same approach for Stmts though.

The difference between Rows and Stmts is that when the rows Interceptor methods (RowsNext, RowsClose) are called the driver.Rows object is passed back as it was created by the user (wrappedRows.parent).
When the Stmt'sExecContext and QueryContext methods are called, the driver.Stmt object from the user is wrapped first into wrappedParentStmt which has additional compatibility methods and is then passed back to the user.
So the user does not get back the same object that he created, he gets a wrappedParentStmt back and he has no access to the field that contains his original Stmt.

Am I misunderstanding something? Or do you see a way how Stmts could be refactored to use the same approach then for rows?

@fho fho closed this May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store user-defined data in Rows, Stmt and Tx objects
3 participants
@fho @tcolgate @inconshreveable