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

Implemented #385 enhancement and updated documentation #549

Merged
merged 4 commits into from
Apr 23, 2021

Conversation

Draco94
Copy link
Contributor

@Draco94 Draco94 commented Mar 25, 2021

Sometimes cx_Oracle may have problems converting data to unicode and you may want to inspect the problem closer rather than auto-fix it using the encodingerrors parameter. This may be useful when a database contains records or fields that are in a wrong encoding altogether.

Signed-off-by: Darko Djolovic <ddjolovic@outlook.com>
@anthony-tuininga
Copy link
Member

Thanks for the suggestion! I think I would prefer this new featue to be on the variable just like encoding errors is -- but I understand your desire to put it on the connection, as that simplifies its use -- no need to create an output type handler function to create the necessary variable. I've been considering creating an "options" object of sorts that would allow simpler configuration of common tasks -- things like returning BLOB as bytes and CLOB as strings, returning numbers as decimal.Decimal objects, etc. This task would be another candidate for that "options" object. The "options" object would be able to be set on the connection (to affect all cursors) or on the cursor (to affect only that cursor). I think that would address this enhancement as well as simplify other common tasks. Thoughts?

@Draco94
Copy link
Contributor Author

Draco94 commented Mar 25, 2021

Agreed that a slightly more elegant way to accomplish this is an output type handler that can be adjusted intelligently to bypass the standard decoding. A philosofical issue is: what is the status of this functionality, a full blown feature or an 'as is' means for troubleshooting certain issues at own risk. I think making this more advanced also implies the 'normality' of the usage of it, as opposed to seeing this as a troubleshooting tool. I think it makes sense to discourage usage of non-uniform encodings as we can't guarantee how Oracle will handle this in the future to start with.
We had to build the patch in our pull request to accomodate our own needs. If you have a more outspoken idea of how you would like this implemented, maybe I can have a look and see if I can do that implementation based on that.

@anthony-tuininga
Copy link
Member

Yes, I agree that we don't want to encourage people to use this feature. :-) So I would suggest adding the parameter to the cursor.var() method and storing the flag on the variable itself, not the connection. That implies that to use it you will need to have an output type handler that creates the variable with the correct parameters -- a little more complex, but not too bad, and certainly doesn't encourage its use! What do you think about that?

@Lexcon
Copy link

Lexcon commented Mar 25, 2021

Currently cursor.var(bytearray) is not supported. So enabling cursor.var(bytearray) will not break anything. We can implement this type and make it assume that it's just a 'passthrough' type that does not do any encode() or decode(). The result will always be a series of bytes that can be handled on the Python level. This way somebody do after-retrieval conversion, whether it's boolean Y or 1 for 'True' and N or 0 for 'False': anything is possible then because this conversion would be done after cx_Oracle is done delivering the results.

Alternatively, a cursor.var() variable can have a pointer to a conversion function on Python level:

def handler(cursor, name, defaultType, size, precision, scale):
__if name == 'MYBOOLEANFIELD': # so a certain boolean field needs conversion:
____def translate_boolean(data_in):
______if data_in == b'Y' or data_in == b'YES':
________return True
______return False
____v = cursor.var(bytearray)
____v.converter = translate_boolean
____return v
__return None

@cjbj
Copy link
Member

cjbj commented Mar 25, 2021

@Draco94 can you point me at your entry in https://oca.opensource.oracle.com/?ojr=contrib-list ?
If you're not there, can you "sign" the OCA? https://oca.opensource.oracle.com/

Without this, we will have to abandon this PR, which would be a shame.

Signed-off-by: Darko Djolovic <ddjolovic@outlook.com>
@Draco94
Copy link
Contributor Author

Draco94 commented Mar 26, 2021

@cjbj I didn't have one but I have submitted form for OCA. It's in 'under review' state. I also made changes to Cursor.var() that @anthony-tuininga was proposing.

Hopefully this change is acceptable, if so I can undo my previous changes and update the documentantion.

The change is that when creating Curson.var() in the output type handler you can pass in additional parameter called bypassstringencoding.
When this flag is set it will override the var -> transformNum to 1 which is for CXO_TRANSFORM_BINARY.

@cjbj
Copy link
Member

cjbj commented Mar 26, 2021

@Draco94 thank you - that's great.

I may not get notified when the OCA is approved; let us know what the outcome is.

@Draco94
Copy link
Contributor Author

Draco94 commented Apr 2, 2021

For your information I resubmitted the OCA request because I noticed I was missing checkmark. I'll let you know when it's approved.

@cjbj
Copy link
Member

cjbj commented Apr 9, 2021

@Draco94 thanks for getting the OCA sorted. I'll let Anthony continue with the tech side.

@Draco94
Copy link
Contributor Author

Draco94 commented Apr 12, 2021

@cjbj Thank you. Hey @anthony-tuininga my second commit is regarding different approach. Please let me know what do you think. If it's okay, I'll remove the other changes from the first commit.

Signed-off-by: Darko Djolovic <ddjolovic@outlook.com>
@Draco94
Copy link
Contributor Author

Draco94 commented Apr 12, 2021

I've removed first commit and left @anthony-tuininga suggestion change with updated documentation. Let me know what do you think.

@cjbj
Copy link
Member

cjbj commented Apr 13, 2021

Ooooh it has doc! :)

@anthony-tuininga
Copy link
Member

@Draco94, the changes look reasonable to me. The name of the attribute (bypassstringencoding) is a bit wordy. Perhaps just bypassencoding is sufficient? I'll give it some more thought. I'm also uncertain if the round trip (using this approach for both insert and fetch) support is complete. Can you add some tests to prove that? That would be helpful.

…sstringencoding' to 'bypassencoding' with updated documentation

Signed-off-by: Darko Djolovic <ddjolovic@outlook.com>
@Draco94
Copy link
Contributor Author

Draco94 commented Apr 21, 2021

@anthony-tuininga Thanks for your feedback. Sample file is: "samples/QueringRawData.py". I also renamed the "bypassstringencoding" to "bypassencoding ". Let me know what do you think.

@anthony-tuininga
Copy link
Member

@Draco94, I think the changes are close enough that I am inclined to accept them and then make any further minor changes directly, rather than ask you to do so. Would that work for you?

@Draco94
Copy link
Contributor Author

Draco94 commented Apr 22, 2021

@anthony-tuininga Yeah, that sounds good to me. Thank you!

@anthony-tuininga anthony-tuininga merged commit 95baec2 into oracle:master Apr 23, 2021
@anthony-tuininga
Copy link
Member

Ok. I've merged this and will make a few adjustments. Thanks for your work on this!

anthony-tuininga added a commit that referenced this pull request Apr 23, 2021
consistent and to comply with PEP 8 naming guidelines; also adjust
implementation of #385 (originally done in pull request #549) to use the
parameter name `bypass_decode` instead of `bypassencoding`.
@anthony-tuininga
Copy link
Member

anthony-tuininga commented Apr 23, 2021

I modified the name of the parameter to be bypass_decode since that is what it is actually doing. The ability to pass values of type bytes to string variables was always present, so nothing needs to be done there! I tweaked the documentation and the sample as well. Take a look and let me know if you have any feedback you'd like to give! Thanks again for your work on this.

@cjbj
Copy link
Member

cjbj commented Apr 24, 2021

@Draco94 thanks for this useful enhancement.

@Draco94
Copy link
Contributor Author

Draco94 commented Apr 26, 2021

@anthony-tuininga, @cjbj I took a look at adjustments and sample file. All looks good and clean to me. Thanks again guys!

@Draco94 Draco94 deleted the 385-enhancement branch April 27, 2021 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants