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

Fix erronous encode of bytes array #598

Merged
merged 3 commits into from
May 7, 2021

Conversation

gschwind
Copy link
Collaborator

Overview

The encode function does not exist for bytes type, thus I guessed it was wrong.

Related Issue / Discussion

Additional Information

Contribution Agreement

(as per https://github.com/geopython/pywps/blob/master/CONTRIBUTING.rst#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to PyWPS. I confirm that my contributions to PyWPS will be compatible with the PyWPS license guidelines at the time of contribution.
  • I have already previously agreed to the PyWPS Contributions and Licensing Guidelines

@coveralls
Copy link

coveralls commented Apr 22, 2021

Coverage Status

Coverage remained the same at 0.0% when pulling 3734f3c on gschwind:pywps-4.4-002 into 559f13b on geopython:pywps-4.4.

@cehbrecht cehbrecht requested review from cehbrecht and huard April 22, 2021 12:49
pywps/inout/outputs.py Outdated Show resolved Hide resolved
@gschwind
Copy link
Collaborator Author

Hello,

I pushed a new logic here d69f1ad.

The logic here is do our best to inline data, i.e. do not encode data to base64, but if it's unsafe to do so then encode it in base64.

@gschwind
Copy link
Collaborator Author

How do you want use regexp ?

@huard
Copy link
Collaborator

huard commented Apr 23, 2021

The detection of existing CDATA tags.

@gschwind
Copy link
Collaborator Author

Updated with regexpr

@huard
Copy link
Collaborator

huard commented Apr 26, 2021

@gschwind
Copy link
Collaborator Author

Hello huart,

Maybe I'm wrong but after digging into the code, I think the code that you are providing is unrelated to the code that we are looking at.

The current code looks miss leading in my opinion, the json code that we are looking here is used for 2 different purposes. It is used to (1) serialize requests into the database and (2) for converting outputs to xml response. In the context of serialization of requests (1), we do not need to store the data of the outputs because they do not exist at all when we serialize those outputs. This is why there is no code to de-serialize the data in the outputs code while it is present in the inputs code. Never the less because of the second purpose of this json code, which is converting the outputs to the xml (2), the code need to serialize actual data of the outputs. This is the code modified in the patch above. This code belong the code used to serialize request (1) to the database but it is not useful for that purpose (1).

I currently thinking of better approach that will make the code more obvious, but I do not have any concrete idea at the moment. If you have any suggestion :)

@huard
Copy link
Collaborator

huard commented Apr 30, 2021

To be clear, my point was to specify the full regexp pattern when looking for an existing CDATA tag. So replace

if not re.search('\\]\\]>', out):

by something like

CDATA_PATTERN = re.compile(r'<!\[CDATA\[(.*?)\]\]>')
if not CDATA_PATTERN.search(out):

But to be fair, it's not clear to me why we need check for an existing CDATA tag in the data in the first place.

Another thing to note is that there is another PR looking at OGC API, where the WPS response is a json file, which would not include a CDATA tag. I'll have some time to look into this next week.

@gschwind
Copy link
Collaborator Author

Hello,

Oh, Ok ! :) To answer why I do not use the full pattern, this is because the aim of the check is to check if it is safe to add the CDATA pattern around the string. This is not safe to to this if the string contain ']]>'. For instance we can't use CDATA pattern for:

"Hello ]]> World"

It is not intend to check if the string already have the CDATA pattern. We may do this check too, in case the user added it already.

Best regards

@huard
Copy link
Collaborator

huard commented Apr 30, 2021

Got it ! Thanks for the clarification, makes sense.

@cehbrecht
Copy link
Collaborator

@gschwind @huard good to merge?

When the patches are merged I can make a final 4.4 release ... and move to the new main branch.

@huard
Copy link
Collaborator

huard commented May 6, 2021

Let me run this with emu+birdy. Should be able to do that today.

Copy link
Collaborator

@huard huard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with emu + birdy.

pywps/inout/outputs.py Outdated Show resolved Hide resolved
Co-authored-by: David Huard <huard.david@ouranos.ca>
@huard
Copy link
Collaborator

huard commented May 7, 2021

@cehbrecht Ok to merge.

@cehbrecht cehbrecht merged commit 9169013 into geopython:pywps-4.4 May 7, 2021
@cehbrecht
Copy link
Collaborator

@gschwind @huard done. Thanks :)

@gschwind gschwind deleted the pywps-4.4-002 branch July 20, 2022 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants