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

Add HttpPoster (#460) #462

Closed
wants to merge 1 commit into from
Closed

Add HttpPoster (#460) #462

wants to merge 1 commit into from

Conversation

dr0i
Copy link
Member

@dr0i dr0i commented Aug 26, 2022

Supersedes #461.

@dr0i dr0i force-pushed the 460-addHttpPoster_ branch 2 times, most recently from 8ff88ab to 9b6d5ba Compare August 26, 2022 09:32
@dr0i dr0i force-pushed the 460-addHttpPoster_ branch from 9b6d5ba to dcff44e Compare August 26, 2022 09:40
@dr0i dr0i requested a review from blackwinter August 26, 2022 09:50
Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

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

Would it make (more) sense to just extend the HttpOpener with more options? (request method, request body, response code range [although I'm a bit hesitant about transporting both success and error data over the same channel, without any means of differentiating them], etc.)

@dr0i
Copy link
Member Author

dr0i commented Aug 26, 2022

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?

@blackwinter
Copy link
Member

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).

@dr0i
Copy link
Member Author

dr0i commented Aug 26, 2022

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?)

@blackwinter
Copy link
Member

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
Copy link
Member

blackwinter commented Aug 26, 2022

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

Maybe this helps to further the discussion.

@dr0i
Copy link
Member Author

dr0i commented Aug 29, 2022

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

@blackwinter
Copy link
Member

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.

@fsteeg
Copy link
Member

fsteeg commented Aug 29, 2022

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.

@blackwinter blackwinter mentioned this pull request Sep 1, 2022
@blackwinter
Copy link
Member

Superseded by #463.

@blackwinter blackwinter closed this Sep 8, 2022
@blackwinter blackwinter deleted the 460-addHttpPoster_ branch September 8, 2022 12:02
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.

3 participants