inspectorG4dget / AnonymousFeedback

1 stars 0 forks source link

DBAPI cursor.execute() invocations should be structured to perform rollbacks on error to prevent error propagation into valid invocations of cursor.execute() #38

Closed scriptbae closed 7 years ago

scriptbae commented 7 years ago

As noted by @NuclearBanane in #36, some pg8000.ProgrammingErrors only appear if a previous cursor.execute() invocation resulted in an error.

These invocations should be structured in such a way that failing cursor.execute() invocations trigger a rollback.

cannotparse commented 7 years ago

If you create a generalized way of handling this, we can make them all uniform after that model.

scriptbae commented 7 years ago

@NuclearBanane this could be handled by wrapping all cursor.execute() calls in some safe_cursor_execute(sql_string, args) function which catches all ProgrammingErrors and invokes a rollback on exception.

It's a quick fix at the cost of structure, but it could be refactored to add granularity as higher-priority issues are resolved and more time can be allotted to this.

cannotparse commented 7 years ago

Its bad practice to have an SQL string unless there was a type wrapping it to prevent use of string format. The current way of doing it, with ?, runs it through a parser to eliminate SQLi.

However, as long as you handle the query strings properly for now, we'll be ok.

Although, I hate the emplace strings. They make the code very "Unpythonic"

scriptbae commented 7 years ago

@NuclearBanane I don't follow. What is SQLi, and what does it have to do with this? We also don't use ? at all in our pg8000 code.

I also don't know what you mean by "emplace strings."

scriptbae commented 7 years ago

After discussion with @inspectorG4dget, applying the adapter pattern may be a cleaner solution to this, and the applicability of this will be assessed after the application is feature complete pre-demo.

A separate issue will be opened for this at a later time.

scriptbae commented 7 years ago

Closed by PR #52.