metafacture-core icon indicating copy to clipboard operation
metafacture-core copied to clipboard

Add HttpPoster (#460)

Open dr0i opened this issue 2 years ago • 8 comments

Supersedes https://github.com/metafacture/metafacture-core/pull/461.

dr0i avatar Aug 26 '22 09:08 dr0i

Why you don't like it? I like the clearness about it (extending an an 'AbstractUrlConnection', not just any HttpOpener). Granted, one more class is introduced , but then I like very much small classes.

hesitant about transporting both success and error data over the same channel

Ok. What's your proposal? Just passing an empty response and an throwing an Exception?

dr0i avatar Aug 26 '22 14:08 dr0i

Granted, one more class is introduced , but then I like very much small classes.

But then you'd have to introduce new classes (and Flux commands) for every other verb we'd like to support (put-http, delete-http, ...). I like flexibility, I guess ;)

Although I realise that HttpOpener and HttpPoster accept different types of input: the URL and the request body, resp.

Maybe we could introduce a placeholder for the input data, e.g. @- as in curl:

"<some url>"
|open-http() # default: method="get", url="@-"
"<some data>"
|open-http(url="<some url>", method="post") # default: body="@-"

Ok. What's your proposal? Just passing an empty response and an throwing an Exception?

I don't really have one right now. Maybe something akin to what we're doing in Fix functions: Let the user control the error case, e.g. with a prefix (that can have a default in this case).

blackwinter avatar Aug 26 '22 14:08 blackwinter

But then you'd have to introduce new classes (and Flux commands) for every other verb we'd like to support (put-http, delete-http, ...).

But I like that, because these commands are soo self explaining (and documentation (i.e. usability) is one of our weaknesses). Many parameters make things so complicated (in my view).

Hm,hm. Maybe you have point. So this boils down to have only one class: HttpOpener, configurable as GET/POST/PUT/DELETE ...

But then, what if we have other classes making use of URLConnection, like https://github.com/linked-swissbib/swissbib-metafacture-commands/blob/master/src/main/java/org/swissbib/linked/mf/source/MultiHttpOpener.java (I guess we could do this reusing HttpOpener) or some SitemapReader (just heard about, don't know if this actually exists (@TobiasNx do you know?)).

[edit: when using other protocols then http(s) an AbstractURLConnection would be handy - but I guess we never will have a usage for other protocols?)

dr0i avatar Aug 26 '22 15:08 dr0i

Many parameters make things so complicated (in my view).

That's hard to argue with. With great flexibility comes great responsibility ;) But then again, we'd have a unified interface instead of different classes with different behaviour and options.

But then, what if we have other classes making use of URLConnection

I'm not sure I follow. If you can subclass AbstractUrlConnection, you can also subclass HttpOpener? [Well, if it weren't for those pesky final classes :( But that's a personal pet peeve of mine. I hate that you can't subclass any Metafacture classes.]

blackwinter avatar Aug 26 '22 15:08 blackwinter

Not to rain on your parade, but this is the direction I had in mind: 3f7e150.

Maybe this helps to further the discussion.

blackwinter avatar Aug 26 '22 17:08 blackwinter

Opened your branch #463. I like it , just added a content type header. Shall we close this branch and further discuss at #463?

dr0i avatar Aug 29 '22 07:08 dr0i

Shall we close this branch and further discuss at #463?

If you think that's the way to go, then yes. But it wasn't my intention to kill the discussion here; you had a valid point with "multiple simple commands" vs. "one complex command". I just wanted to illustrate my point of view.

blackwinter avatar Aug 29 '22 09:08 blackwinter

some SitemapReader (just heard about, don't know if this actually exists (@TobiasNx do you know?))

Yes, it's in oersi-etl (impl, test) and we have a card in our sprint backlog to move it to metafacture-core.

fsteeg avatar Aug 29 '22 11:08 fsteeg

Superseded by #463.

blackwinter avatar Sep 08 '22 07:09 blackwinter