search

amphp / postgres

Async Postgres client for PHP based on Amp.
MIT License
97 stars 20 forks source link

Improper getCurrent implementation for end of ResultSet #31

Closed Bilge closed 3 years ago

Bilge commented 4 years ago

https://github.com/amphp/postgres/blob/54186c9d262910075fe6cbd3cc01e45f774f6759/src/PgSqlResultSet.php#L76-L78

I'm not sure this is a good way to implement getCurrent(). It is the role of advance() to move the cursor forward. If it's at the end, I think it should just continue emitting the same row, rather than throwing an Error.

Moreover, since this is a generic type \Error, it's difficult to catch this without the risk of including false positives. I think we should either avoid throwing errors in this situation, or double down and throw a specific error type from the library's namespace.

trowski commented 4 years ago

Calling getCurrent() after advance() resolves to false is undefined, so throwing an error seems appropriate.

This issue will go away in the v2 release, as result sets will be switching to implementing Stream.

trowski commented 3 years ago

Do note that getCurrent() returns the current row unless it's null, at which point the next row is fetched. Calling getCurrent() by itself does not advance the result.

The implementation is still not ideal, but will be improved for v3, as Streams got pushed from Amp v2 to v3.