PE-5845 NC - Creating a group with diacritical marks (áéíóúäëïöüñçâê)

[MichalBryxi]

This allows us to buffer HTTP requests so that they can be read multiple times.

Only supportive PR for the ticket above.

[cprice404]

This breaks the existing tests, and also it would be good to add some test coverage for this functionality if possible.

Also, do you know whether there would be a downside to simply always wrapping the result in a BufferedEntity? If that didn't cause problems and would allow us to forego the introduction of a new setting, that would be preferable. If we do have to introduce the new setting we'll probably want to check and see if something similar exists in the clj-http library, and try to choose a name that is consistent with whatever they've called it.

[fhrbek]

@cprice404 I don't think it would be smart to always support entity buffering as it would use a lot of resources on sending huge uploads for example. The only use case I can imagine it makes sense is when you want a proxy to follow redirects even on PATCH, PUT and POST requests, only for small data.

It looks like the clj-http library does support this, the option is called entity-buffering (see https://github.com/dakrone/clj-http/blob/master/src/clj_http/client.clj#L403). So, perhaps we should use the same option name, but it should default to false IMHO.

[cprice404]

@fhrbek thanks for the info. I agree with your assessment, let's just change the name to match theirs, and resolve the test issues then.

[MichalBryxi]

In the end we found that the logic can be simplified a bit. We use buffering when force-redirects is set to true and when HTTP methods which supports body is used.

Tests are passing now. We think that it's not necessary to introduce new tests. There are already tests which covers cases with force-redirects.

[cprice404]

This looks like it needs a rebase now.

[MichalBryxi]

Rebased, thanks for noticing.

[cprice404]

@MichalBryxi Thanks. BTW is this blocking integration work? Most of us are kind of frantically getting ready for PuppetConf today, so we might wait and pick this up when we get back next week... but if that's going to cause you a lot of grief then we'll try to get to it sooner.

[MichalBryxi]

@cprice404 No worries here. I think this can wait. It blocks only one bug.

[cprice404]

Great, thanks. We'll put it at the top of the to-do list for after the conference.

[MichalBryxi]

Can we look at this now? @cprice404

[camlow325]

@MichalBryxi Could you provide a little more information as to exactly what the benefit of this PR would be?

If this is intended to address the encoding issues discussed in PE-6019, I wonder if the real source of the problem is the same as that for a recently fixed issue, PE-6019 (https://github.com/puppetlabs/clj-http-client/pull/17). That fix was rolled out in the clj-http-client 0.2.7 release. Before that change, a string entity body with a header like "Content-Type:application/json; charset=UTF-8" would be mistakenly encoded as ISO-8859-1 instead of UTF-8. With the change for the 0.2.7 release, the string entity body would be encoded per the charset specified in the Content-Type header. Can you verify if this issue is still present when using clj-http-client 0.2.7?

[fhrbek]

@camlow325 You're right that the original issue that we had (encoding) is probably fixed with PE-6019. However, if you happen to get a body as an instance of InputStream and you don't make it a buffered input stream, you'll probably get an error on an attempt to proxy a post/put/patch request with forced redirects because the stream will get exhausted in the original endpoint, then the request will be redirected to a new location and the input stream won't be available any more. That's why I think this PR should really be merged.

[MichalBryxi]

@camlow325 Tested pe-classifier-ui project with http-client-0.2.7 and I can confirm that this bug (PE-5845) is fixed. I get correct characters back from the service when I tested it. But I guess @fhrbek has a point that the problem with "read" stream might occur.

[camlow325]

@fhrbek I've done a little bit of testing with :force-redirects and :follow-redirects "true" and PATCH, PUT, and POST requests through the Clojure binding in our library. With or without the proposed BufferedHttpEntity change, I see the same behavior:

• For a POST request, the client issues a GET request when following the redirect. The GET request has no request body.
• For a PUT or PATCH request, the client does not follow the redirect -- no follow-up request issued in response to the HTTP 3XX response to the initial request.

I won't claim that the above behaviors are the desired behavior. Given the above, though, I don't see any immediate benefit to including the proposed change for this PR. It may make sense to file a separate JIRA ticket to investigate the desired redirect support that the library should provide for PATCH, PUT, and POST requests.

Are there any other cases where you can demonstrate that this change has a positive effect? If so and you can add that to an automated unit test as part of this PR, I'd be open to merging it. What do you think?

[fhrbek]

@camlow325 I agree that we can file a new ticket and explore the situation a little bit. I believe we don't need this PR critically now for the encoding problem, so it can wait if all current tests pass and there's nothing known that would break any PE features. However, what you describe is surprising - perhaps something has changed recently. We were actually forced to use the buffered input stream because otherwise we ran into errors with the input stream reset on post/put/patch requests with force redirects set to true (it was when there was the sting slurp instead of current http entities), and we were even unable to run all unit tests successfully when we introduced data streaming instead of slurping the string. With force-redirects set to true, the data was certainly sent to the redirected location, and even the method was preserved. As I was helping @MichalBryxi to resolve the original issue I'm curious how it works now and how safe it is in all possible situation. I'll be a volunteer on the potential ticket, at least as a co-working developer. It's possible that after changing the logic it can't happen that the data is ever resent to the redirected location (which might be correct and the former behavior could have been actually a bug).

[camlow325]

With force-redirects set to true, the data was certainly sent to the redirected location, and even the method was preserved.

@fhrbek Thanks, I haven't done much with this library and redirects so that's good to know. I would like to know if you are able to reproduce the behaviors that I'm seeing on the latest code and also if there was a specific past version where the redirects seemed to be forwarding the content properly. It may be that we unintentionally introduced a regression at some point, not sure.

[cprice404]

OK, so, if I'm reading everything above correctly, this PR isn't known to be blocking anything for PE3.4? If that's the case, can we close it out for now and revisit later when we have more bandwidth?

[cprice404]

same question for puppetlabs/ring-middleware#14

[fhrbek]

@cprice404 From my point of view, yes, this can be closed for now, with a follow-up ticket entered (not done yet AFAIK).

[cprice404]

ok, thanks!

[camlow325]

fyi, I created TK-90 to cover the redirection issues for PUT, PATCH, and POST issues.