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

Do not append missing XML declaration in formatted responses #1183

Closed
wants to merge 7 commits into from

Conversation

this-is-r-gaurav
Copy link
Contributor

@this-is-r-gaurav this-is-r-gaurav commented Oct 15, 2021

Fixes #1156.

@jkbrzt jkbrzt self-requested a review October 15, 2021 06:47
@@ -43,17 +43,22 @@ def format_body(self, body: str, mime: str):

from xml.parsers.expat import ExpatError
from defusedxml.common import DefusedXmlException

pretty_body = body
Copy link
Contributor

@BoboTiG BoboTiG Oct 15, 2021

Choose a reason for hiding this comment

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

I would do all the work into the else: block, see my next comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it would be more appropriate, i missed that part.

try:
parsed_body = parse_xml(body)
except ExpatError:
pass # Invalid XML, ignore.
except DefusedXmlException:
pass # Unsafe XML, ignore.
else:
body = pretty_xml(parsed_body,
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps something less invasive could be used? An idea:

        try: ...
        else:
            has_declaration = body.startswith('<?xml version')
            body = pretty_xml(parsed_body,
                              encoding=parsed_body.encoding,
                              indent=self.format_options['xml']['indent'],
                              standalone=parsed_body.standalone)
            if not has_declaration:
                body = body[body.index('\n') + 1:]

         return body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes seems more intuitive. I was not aware about the ordering of prolog attributes matter. so i choose to find out what is the first tag in original xml document.
But i tried one test case:
If unformatted file is this

<?xml version="1.0"?>
<root><test>testValue</test></root>

what would be expected, this

<?xml version="1.0"?>
<root>
  <test>testValue</test>
</root>

or

<?xml version="1.0" encoding="utf-8"?>
<root>
  <test>testValue</test>
</root>

Copy link
Contributor

@BoboTiG BoboTiG Oct 18, 2021

Choose a reason for hiding this comment

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

xmlminidom will set the encoding. I think it is good as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the previous answer. I did a mistake.
HTTPie should not alter the response content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay, sorry been busy with personal work will try to fix that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BoboTiG addressed the above test case as well.
But assuming if test case file is:

<root><test>testValue</test></root>

Output will be

<?xml version="1.0" ?>
<root>
  <test>testValue</test>
</root>

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that input, the output should be:

<root>
  <test>testValue</test>
</root>

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add tests for different cases also 🙏 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I will add them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not Adding test case

<root><test>testValue</test></root>

because it can be addressed by simple_raw.xml

Gaurav and others added 2 commits October 18, 2021 20:58
Co-authored-by: Mickaël Schoentgen <contact@tiger-222.fr>
@this-is-r-gaurav
Copy link
Contributor Author

@BoboTiG
Copy link
Contributor

BoboTiG commented Oct 26, 2021

Let's see with @jakubroztocil.

@isidentical
Copy link
Contributor

@this-is-r-gaurav thanks for the PR, but we decided to with another way (#1227).

@this-is-r-gaurav this-is-r-gaurav deleted the fix-for-1156 branch December 26, 2021 05:29
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.

Do not append missing XML declaration in formatted responses
3 participants