metafacture-core
metafacture-core copied to clipboard
metafacture
Extend `HttpOpener`.
re "tests": we didn't have these before. Are they really necessary? Could you go on provide those?
we didn't have these before. Are they really necessary?
Exactly. And I honestly don't think that's a good argument ;) The functionality is getting more complex and we should have at least some test coverage.
Could you go on provide those?
Yes, I could, but probably not too soon.
I know the following is also not a good excuse - but can we merge this already because we have some issues to solve? Tests would be only postponed - promised!
I know the following is also not a good excuse - but can we merge this already because we have some issues to solve?
Of course, I don't want to hold back progress. But we shouldn't rush anything prematurely, either. Are you sure that the design is sound? Do others want to chime in? I mean, there were some concerns regarding complexity and reusability. (Will you change the content-type default handling?)
How would this be consumed, anyway? Are you going to publish a new release?
Tests would be only postponed - promised!
Haven't we all heard that one a thousand times? ;)
How would this be consumed, anyway? Are you going to publish a new release?
Would be the basis for https://github.com/metafacture/metafacture-core/issues/464 and that one to solve https://gitlab.com/oersi/oersi-etl/-/issues/64.
As long as there is no API break introduced (i.e. nothing that is already released would be broken) I am slack @fsteeg may want to throw 2 cents?
But how would you consume the updated version after the merge? Aren't you running from a checked out branch anyway?
I'm not saying it would be better for you to change the branch in your build, but would it be an option? Would you consider it preferable to merging this "unfinished" pull request?
As long as there is no API break introduced (i.e. nothing that is already released would be broken)
How confident are we about that without any tests? ;)
How confident are we about that without any tests? ;)
as we didn't have a test before we cannot test against anything. Besides some flux scripts which does API calls and manually check if the old ones do work as before. And yes, proper tests would be better of course.
I'm not saying it would be better for you to change the branch in your build, but would it be an option?
yes, it would. Ok, so I am going to use this PR at it is to implement agains it. (sorry for the noise, just getting a bit impatient ;) )
yes, it would. Ok, so I am going to use this PR at it is to implement agains it.
Okay, thanks :)
(sorry for the noise, just getting a bit impatient ;) )
I know the feeling all too well ;)
as we didn't have a test before we cannot test against anything.
Whatever new tests we devise, we can run against the previous implementation. But until then we're in the dark, yes.
While spiking out an approach for unit tests, I noticed that URLConnection.setDoOutput(true) changes a GET request to POST. I.e., we can't initiate a GET request with a payload body.
Is this something we need to be able to support? It's technically undefined, but quite common with Elasticsearch for instance.
@dr0i: Would you review the pull request? Can't seem to request a review from the submitter.
can't initiate a GET request with a payload body.
As an example you referenced Elasticsearch, but then Elasticsearch also allows POST, so we can stick to POST. So IMO we don't need to support "Get with payload body".
Yes, Elasticsearch also supports POST. It's just that it might be unexpected if you initiate a GET request that then gets silently converted to something else (as a practical example: If your Elasticsearch sits behind a firewall that only allows GET, your search request fails for no apparent reason).
It's only an issue with server implementations that behave differently for GET with body vs. POST.
Re: "GET with body vs. POST" : I understand that we can implement later, if need really arises, get a Response Body doing a GET, without breaking the API.
Yes, I think we should proceed with the current implementation. I was just surprised by this behaviour and wanted to make sure we're on the same page.
Re. GET with payload body, I like this post which made me aware of the HTTP QUERY Method (current draft). The post mentioned writes:
This is a new method, that’s not quite standard yet but any compliant HTTP client and server should already support it.
For rhe reconciliation API I just created an issue to track this. We should probably seriously consider supporting QUERY in the HTTPOpener as well.
We should probably seriously consider supporting QUERY in the HTTPOpener as well.
We're currently limited to the request methods that HttpURLConnection.setRequestMethod() supports. If we need other methods or generally more flexibility, we'd have to replace it with a different implementation.