Use POST method for getting pileup documents in MSTransferor

[amaltaro]

Fixes #11935
Complement to #11910

Status

ready

Description

The title says it all, use the POST instead of GET method for retrieving pileup documents with a specific query string.

Is it backward compatible (if not, which system it affects?)

YES

Related PRs

None

External dependencies / deployment changes

None

[cmsdmwmbot]

Jenkins results:

• Python3 Unit tests: succeeded
• Python3 Pylint check: succeeded
  • 4 warnings
  • 47 comments to review
• Pylint py3k check: succeeded
• Pycodestyle check: succeeded
  • 19 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/14976/artifact/artifacts/PullRequestReport.html

[cmsdmwmbot]

Jenkins results:

• Python3 Unit tests: failed
  • 1 new failures
  • 2 changes in unstable tests
• Python3 Pylint check: succeeded
  • 4 warnings
  • 47 comments to review
• Pylint py3k check: succeeded
• Pycodestyle check: succeeded
  • 19 comments to review

Details at https://cmssdt.cern.ch/dmwm-jenkins/view/All/job/DMWM-WMCore-PR-test/14977/artifact/artifacts/PullRequestReport.html

[d-ylee]

Looks good to me. Thanks Alan.

[amaltaro]

Thanks Dennis!

[vkuznet]

@amaltaro , @d-ylee , it is pity that it is already merged. I was away for two days and haven't had time to provide my input. The issue you fixed with switching to POST goes against HTTP standard which mandate that POST requests must be used for creation of resources and not for data retrieval which is reserved for GET requests.

To manage JSON data you can still use GET, and here is a proof using Go code (which is much more intuitive and easier to demonstrate for this issue):

# server code
package main

import (
    "io"
    "log"
    "net/http"
    "net/http/httputil"
)

func RequestHandler(w http.ResponseWriter, r *http.Request) {
    log.Printf("URI %+v Header: %+v TLS: %+v", r.RequestURI, r.Header, r.TLS)
    if r.Header.Get("Accept") == "application/json" {
        data, err := io.ReadAll(r.Body)
        if err == nil {
            w.Write(data)
        } else {
            w.Write([]byte(err.Error()))
        }
        return
    }
    if req, err := httputil.DumpRequest(r, true); err == nil {
        w.Write(req)
    }
}

func main() {
    http.HandleFunc("/predict", RequestHandler)
    port := ":8888"
    log.Println("Start GoFake HTTP server on port", port)
    http.ListenAndServe(port, nil)
}

and if I compile it and run, e.g. go build fake.go; ./fake it will run on port 8888. Now the client can do either POST or GET:

# using POST HTTP request
curl -X POST -H "Content-type: application/json" -d '{"foo": 1}' http://localhost:8888/predict
POST /predict HTTP/1.1
Host: localhost:8888
Accept: */*
Content-Length: 10
Content-Type: application/json
User-Agent: curl/8.6.0

{"foo": 1}

# using GET HTTP request
curl -X GET -H "Content-type: application/json" -d '{"foo": 1}' http://localhost:8888/predict
GET /predict HTTP/1.1
Host: localhost:8888
Accept: */*
Content-Length: 10
Content-Type: application/json
User-Agent: curl/8.6.0

{"foo": 1}

So, both requests properly shows the payload which is parsed and printed by the server. This proofs that using JSON payload works just fine with HTTP GET requests. Does Python (and in particular our WMCore web server) can handle this properly is an open question but we should not break HTTP standard. Such logic break logic of APIs and their intend. I hope we can re-open this issue to switch back to GET request (but it requires testing) or create another issue to address this.

[amaltaro]

@vkuznet Valentin, the actual problem is when the request with a JSON (actually python dictionary) gets expanded to the URL that we want to access, and this is not represented in your example above, as you are querying one specific endpoint without any query string, as it is required in some cases of MSPileup.

In addition to that, it looks like you have missed this fundamental standard of HTTP method types and the actual actions, given that you implemented it here:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/MicroService/MSCore/MSManager.py#L401-L409

with either resource creation or resource retrieve, based on a MongoDB query.

Last but not least, if you really think this needs to be refactored, I would suggest to go with a new ticket (given that refactoring goes beyond of the changes provided here) and we work on that in the near future. Right now we are stuck and very late with validating and rolling things to production, I'd rather not have yet another blocker on this.

[vkuznet]

Alan, I see two issues here:

1. we should list all end-points and their queries, where parameters should be passed and where JSON, do we need combination of those or not. Then, we can go to next step
2. Identify and properly adjust end-points to handle proper API calls

In iniital implementation of MSCore/MSPileup I provided basic end-points but I don't recall that we had full list of all possible use-cases. This is why we now address this. For instance, using GET method it is totally fine to combine parameters and JSON payload, e.g. here is an example of GET query using both using my previous example:

curl -X GET -H "Content-type: application/json" -d '{"foo": 1}' "http://localhost:8888/predict?param=123"
GET /predict?param=123 HTTP/1.1
Host: localhost:8888
Accept: */*
Content-Length: 10
Content-Type: application/json
User-Agent: curl/8.6.0

{"foo": 1}

Therefore, I suggest to open a ticket with outlining use-cases and end-point usage, then we can adjust the code to properly work with end-points for all different use-cases.