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

Expose the underlying statement in DBAL statements #3534

Open
stof opened this issue Apr 29, 2019 · 18 comments
Open

Expose the underlying statement in DBAL statements #3534

stof opened this issue Apr 29, 2019 · 18 comments

Comments

@stof
Copy link
Member

stof commented Apr 29, 2019

Feature Request

Q A
New Feature yes
RFC yes
BC Break no

Summary

The DBAL Statement object exposes the API for the common abstraction. But the different platforms may have additional features, which can be useful when you know that you are writing code targetting them.
As DBAL 2.x uses inheritance for PDOStatement, this is actually already possible for PDO-based drivers (not for others). As DBAL 3.x moves to composition, we are loosing that possibility. It would be great to add methods in the different Statement implementations returning the underlying statement. This method would not be part of the interface, as its return type would depend on the implementation being used.

For instance, PDO has a PDOStatement::getColumnMeta method to get some info about the type and name of the column in the result, which is very useful when you need to know the name of columns even when the result set is empty. #2176 is asking to bring this particular case to the common abstraction, but being able to escape the abstraction when you know that you use PDO is a good solution in the meantime.

@morozov
Copy link
Member

morozov commented Apr 29, 2019

While it's technically possible to introduce ResultStatement::getColumnMeta() and implement it in some supported drivers, I agree that exposing the underlying statement may be more helpful to the projects which rely on platform-specific features.

Any objections @doctrine/doctrinecore?

@ostrolucky
Copy link
Member

no objections

@Ocramius
Copy link
Member

Giving access to the underlying statement is acceptable in my opinion, but the API needs to be marked as risky for usage, since the return type is mixed (including null for platforms that do not have a statement abstraction). Not sure if @internal would be the correct marker, as we can't guarantee BC on it.

@stof
Copy link
Member Author

stof commented Apr 29, 2019

@Ocramius my suggestion is to have implementation-specific methods, not part of the interface. Then, the return type is not mixed. Then, using it in a type-safe way would imply a check that your statement is a \Doctrine\DBAL\Driver\PDOStatement for instance, but that's expected as these methods would be meant to be driver-specific.

I don't think @internal is the right marker, as that would imply that external packages should not use it, but then it is useless (as Doctrine itself won't use it).

@morozov
Copy link
Member

morozov commented Dec 12, 2021

Linking #5037 as an example of the same requirement implemented for connections.

@stof
Copy link
Member Author

stof commented Dec 13, 2021

Note that with the split of Statement and Result in the DBAL 3.x API, we would need this on the Result object (accessing PDOStatement::getColumnMeta makes sense when dealing with a result). I'm not sure whether there is a use case for accessing the native statement from the DBAL Statement object (at least for PDO, the methods of PDOStatement that are not exposed through the DBAL API are all related to the result side AFAICT)

@yesdevnull
Copy link

I'd like to add my $0.02 if I may, being able to access the PDOStatement from the Result object would be extremely useful in the rare occasions I need to deal with multi-rowsets.

I empathise with Ocramius when he says "the AL in DBAL is no longer useful, but rather a hindrance." however the convenience of using a single, common API for building and executing queries is a huge bonus and would save me having to re-implement some of the logic in Connection::executeQuery in my own repository somewhere.

I imagine this would involve implementing DriverResult::getStatement along with Result::getUnderlyingStatment to facilitate returning the PDOStatement from the DriverResult through the Result object.

Totally understand that this is an edge-case and could be done differently by me, I just wanted to offer an opinion as an end-user of the library.

Thanks Doctrine team!

@derrabus
Copy link
Member

What we do have now is Connection::getNativeConnection() which basically allows you to opt out of the DBAL entirely and operate on the PDO connection for the cases you've mentioned. Would that work for you?

@stof
Copy link
Member Author

stof commented Mar 30, 2023

@derrabus when migrating to DBAL 3, I had to migrate to this. But this definitely made the code less good:

  • I loose the integration with the logger (and the Symfony web debug toolbar) for those executed statements
  • I have to use the PDO statement API to execute the query, which is less nice than the DBAL one regarding parameter binding
  • I loose the support for DBAL parameter types.

In my case, I needed the PDOStatement only to access columnCount and getColumnMeta to be able to automatically build the header line of my CSV response.

Having Result::getNativeResult would have been a lot easier for that case.

@derrabus
Copy link
Member

In my case, I needed the PDOStatement only to access columnCount and getColumnMeta to be able to automatically build the header line of my CSV response.

Would it be possible to create portable versions of both methods on DBAL's result class? Before I expose the internal state of our result object (and that's what the native result is), I'd like to try that path first.

@stof
Copy link
Member Author

stof commented Mar 30, 2023

Well, I'm not sure other drivers would have the equivalent feature (I never investigated it). And it would require making the array shape returned for the meta portable (and to be honest, I'm not even 100% sure that it is fully portable between PDO drivers)

@yesdevnull
Copy link

What we do have now is Connection::getNativeConnection() which basically allows you to opt out of the DBAL entirely and operate on the PDO connection for the cases you've mentioned. Would that work for you?

It would, however like @stof mentioned I would either lose the benefits of Connection:executeQuery or have to re-implement it myself and return a DriverResult instead of Result (even that wouldn't be perfect as I still need to get the underlying PDOStatement from the DriverResult, which is currently private).

I need PDOStatement::nextRowset() and PDOStatement::closeCursor() for interacting with my MSSQL multi-rowsets. I understand that those aren't available in all databases supported by PDO/Doctrine so it could make the driver interface and implementation a bit messy given the lack of feature support across the various databases (and without having to resort to noops on the unsupported driver classes).

My current workaround is... absolutely shameful, I'm using reflection to access private properties on the two result objects. Being able to get the PDOStatement from the DriverResult through the Result object would be the most convenient for me, but that's me being selfish 😁

@derrabus
Copy link
Member

And it would require making the array shape returned for the meta portable

It would require making the information you extract from that array shape portable, yes.


I need PDOStatement::nextRowset() and PDOStatement::closeCursor() for interacting with my MSSQL multi-rowsets.

Do you think we could build a portable abstraction of a multi-rowset result?

I understand that those aren't available in all databases supported by PDO/Doctrine

Is that a problem? A driver that doesn't support it could either throw or return a multi-rowset result with only a single rowset.

My current workaround is... absolutely shameful

And absolutely brittle because it would break the moment a middleware is used that decorates results or we decide to rename private properties in a new release, or… or… or.

Being able to get the PDOStatement from the DriverResult through the Result object would be the most convenient for me

Yes, but it's a leaky abstraction. Let's avoid that if we can find a better way.

@stof
Copy link
Member Author

stof commented Mar 30, 2023

Looking quickly, here is what I found for the column meta API:

  • ext-sqlite3 has SQLite3Result::numColumns, SQLite3Result::columnName and SQLite3Result::columnType (returning an integer corresponding to some constants)
  • PDO has PDOStatement::columnCount and PDOStatement::getColumnMeta (returning a shape with many keys and potentially not implemented by custom PDO drivers)
  • ext-pgsql has pg_num_fields, pg_field_name and pg_field_type
  • ext-mysqli has mysqli_result::$field_count to the field count and mysqli_result::fetch_field_direct to get info about the field
  • ext-ibm-db2 has db2_num_fields, db2_field_name and db2_field_type (and a few other db2_field_* functions)
  • ext-oci8 has oci_num_fields, oci_field_name and oci_field_type
  • ext-sqlsrv has sqlsrv_num_fields and sqlsrv_field_metadata (which gives the metadata for all fields as a list instead of taking an index as argument like for other extensions)

So for the field name and field type of the result, we could add them in the abstraction.

@stof
Copy link
Member Author

stof commented Mar 30, 2023

Note that even DoctrineBundle needed special handling for MSSQL when explaining a query in the profiler because of this multi-rowsets.

Having an escape-hatch for the result API would be consistent with the one we have for the connection one.

@stof
Copy link
Member Author

stof commented Mar 30, 2023

I opened #5980 about introducing the DBAL abstraction for metadata about result columns. This would solve one use case that currently requires accessing the underlying statement.

@stof
Copy link
Member Author

stof commented Mar 30, 2023

@yesdevnull PDOStatement::closeCursor is actually already exposed. That's what the PDO-based drivers are using to implement Result::free

@yesdevnull
Copy link

yesdevnull commented Mar 30, 2023

I need PDOStatement::nextRowset() and PDOStatement::closeCursor() for interacting with my MSSQL multi-rowsets.

Do you think we could build a portable abstraction of a multi-rowset result?

Definitely, just depends if it's worth the extra effort for such a small use case. From reading Ocramius' comment "Unless this is a cross-platform feature, this should not land in DBAL", I wasn't sure how likely it would be for multi-rowset specific handling to land in Doctrine. I am happy to offer any assistance I can, particularly around SQLSrv specific cases.

My current workaround is... absolutely shameful

And absolutely brittle because it would break the moment a middleware is used that decorates results or we decide to rename private properties in a new release, or… or… or.

100%, I'm not justifying my workaround in any way. It's for an infrequently used feature in a project I'm working on, so I can sort of justify the reflection hack. I wouldn't normally condone this unless absolutely necessary.


@yesdevnull PDOStatement::closeCursor is actually already exposed. That's what the PDO-based drivers are using to implement Result::free

Right you are, thanks for pointing that out. I had been looking at the SQLSrv\Result class, which incidentally has the note "deliberately do not consider multiple result sets, since doctrine/dbal doesn't support them" in its implementation of Result::free (which I gather is more of an FYI rather than a statement of intent to never support it).


Appreciate your time and comments, @stof and @derrabus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants