Replace poor man's relation name quoting with implementation from sqlalchemy-cratedb
#38
Conversation
src/commons_codec/model.py
Outdated
| try: | ||
| from sqlalchemy_cratedb.support import quote_relation_name | ||
| except ImportError: # pragma: no cover | ||
| quote_relation_name = None |
There was a problem hiding this comment.
Is this a good pattern as without quoting it can lead to unexpected behaviour (wrong names used, maybe even deleting wrong data/tables)?
There was a problem hiding this comment.
Initially, I didn't want to make sqlalchemy-cratedb, where the support function is now homed, a strict dependency of commons-codec, being originally conceived as a library with minimal dependencies. It is probably a wrong decision, and I will just add it.
We can't vendorize it in a standalone fashion, because it needs the dialect. In this spirit, decoupling them is probably just not viable, and, as said, just adding it, because the focus is about CrateDB anyway, is probably the right decision. Do you agree?
There was a problem hiding this comment.
Yes I think we must depend on it, unexpected behaviour is worse than this dependency ;)
There was a problem hiding this comment.
I've amended the patch according to you suggestions, it is much more straight-forward now. Thank you!
src/commons_codec/model.py
Outdated
| if name and '"' not in name: | ||
| name = f'"{name}"' | ||
| return name | ||
| identifier = f"{self.schema}.{self.table}" |
There was a problem hiding this comment.
As schema and table are already separated here, quoting them dedicated would be much safer.
There was a problem hiding this comment.
Our quote_relation_name function currently only accepts a single identifier. Maybe add another one, which accepts the fragments separately?
There was a problem hiding this comment.
Normally, a quoting function is called per identifier, so for both, schema and name separately. With quote_relation_name we already moved into the direction of supporting a full relation name FQN, that's why we named it like that if I remember correctly. We did this afaik as at some usages, schema and name weren't available separately. Yeah maybe leave it like that, just keep in mind that quoting single identifiers is less error prone than quoting an already FQN relation name.
There was a problem hiding this comment.
Yeah, I had it in mind. In this case, I was just trying to assemble the bricks we have to solve the problem at hand. Every other suggestion is super valuable to provide a rock solid reference implementation to be re-usable for different kinds of applications and use cases.
If you think it is acceptable for now, I will leave it for this iteration, and then, on another one, improve that at crate/sqlalchemy-cratedb#155 going forward. Thanks!
src/commons_codec/model.py
Outdated
| @@ -11,28 +12,25 @@ | |||
| else: | |||
| from backports.strenum import StrEnum # pragma: no cover | |||
|
|
|||
| try: | |||
| from sqlalchemy_cratedb.support import quote_relation_name | |||
There was a problem hiding this comment.
Under what circumstances will this import cause an ImportError? if that happens, TableAdress.fqn() will cause a runtime error TypeError: 'NoneType' object is not callable wouldn't it?
There was a problem hiding this comment.
Yeah, unfortunate code stacking here, see #38 (comment). Let's get rid of code smells completely on behalf of another iteration. Thanks!
There was a problem hiding this comment.
I've amended the patch according to you suggestions, it is much more straight-forward now. Thank you!
... from `sqlalchemy-cratedb` package.
amotl
force-pushed
the
better-tablename-quoting
branch
from
d42fdad to
fda0e4e
Compare
amotl
changed the title
sqlalchemy-cratedb
About
When needed, usequote_relation_namefromsqlalchemy-cratedbpackage. Otherwise, don't quote at all, and push this responsibility to the caller. I think it is better doing it this way, than doing it wrong.Instead of doing it insufficiently, let's pay the price on pulling in the
sqlalchemy-cratedbpackage as a dependency, to be able to use the canonical implementationquote_relation_namewithout further ado.quote_relation_namesupport utility function sqlalchemy-cratedb#155