PecanProject / pecan

The Predictive Ecosystem Analyzer (PEcAn) is an integrated ecological bioinformatics toolbox.
www.pecanproject.org
Other
199 stars 230 forks source link

Update API endpoint URLs #3283

Closed Sweetdevil144 closed 2 months ago

Sweetdevil144 commented 2 months ago

This Fix is similar to this issue in our rpecanapi Repository : https://github.com/PecanProject/rpecanapi/issues/21

Description

Refracted server URLs to use localhost PEcAn URLs

Motivation and Context

Review Time Estimate

Types of changes

Checklist:

infotroph commented 2 months ago

@Sweetdevil144 can you give some detail on how you tested this? I ask because this PR broke the test pathway I'm aware of, but maybe it enables another one I didn't know about?

The pathway I know about is test_pecanapi.sh, which seems to be written to be run inside the API docker image, where localhost:8000 will be the correct name -- note that test_pecanapi.sh calls R/entrypoint.R, which hardcodes port 8000. When you're running the whole Docker stack traefik then does some magic to map that container's port 8000 to the port 80 interface where we see it in normal use. I can't get test_pecanapi.sh to work with these changes applied, but if there'e another way to run it and see tests passing then let's document that.

Sweetdevil144 commented 2 months ago

Sure. Apologies I forgot to test this in hurry. This won't happen again and I assure you of this. Although talking about some breaking changes, I think some of our pre-existing test cases may be either false or returning wrong output. After redirection to pecan.localhost/api/models, our system does not requires auth to do so. Hence, causing failures of following test cases.

test_that("Using incorrect username & password returns Status 401", {
  res <- httr::GET(
    "http://pecan.localhost/api/models/",
    httr::authenticate("wrong_username_user", "wrong_password_user")
  )
  expect_equal(res$status, 401)
})

test_that("Not using username & password returns Status 401", {
  res <- httr::GET(
    "http://pecan.localhost/api/models/",
  )
  expect_equal(res$status, 401)
})
#

Below is a response res I got when I tried manually doing the same thing :

> res <- httr::GET(
+     "http://pecan.localhost/api/models/",
+   )
> res
Response [http://pecan.localhost/api/models/]
  Date: 2024-04-05 13:34
  Status: 200
  Content-Type: application/json
  Size: 3.58 kB

> res <- httr::GET(
+     "http://pecan.localhost/api/models/",
+   )
> res
Response [http://pecan.localhost/api/models/]
  Date: 2024-04-05 16:20
  Status: 200
  Content-Type: application/json
  Size: 3.58 kB
# Even though no username and password are provided, we still see a status returned 200. I tried navigating to this page and found a systematic api response too without any authentication.
> 

Is this a general error or should this be working as defined previously?

I will test all the responses thoroughly and debug the errors too.

Sweetdevil144 commented 2 months ago

This test takes too long to run. In short, the suites don't pass over this test :

test_that("Calling /api/inputs/{input_id} with valid parameters returns Status 200", {
  res <- httr::GET(
    paste0("http://pecan.localhost/api/inputs/", 99000000003),
    httr::authenticate("carya", "illinois")
  )
  expect_equal(res$status, 200)
})
Sweetdevil144 commented 2 months ago

The pathway I know about is test_pecanapi.sh, which seems to be written to be run inside the API docker image, where localhost:8000 will be the correct name -- note that test_pecanapi.sh calls R/entrypoint.R, which hardcodes port 8000.

I'm taking further look into this

Sweetdevil144 commented 2 months ago

Conclusion

I think we would be better off with Partially reverting our changes from pecan.localhost to localhost:8000 with following points in consideration :

Below is a description of all test cases which have been causing problems with current Repository changes. (I plan to revert changes from pecan.localhost back to localhost:8000 and then confirm it once again whether these runs are successful or not) :

   test_that("Using incorrect username & password returns Status 401", {
  res <- httr::GET(
    "http://pecan.localhost/api/models/",
    httr::authenticate("wrong_username_user", "wrong_password_user")
  )
  # return 200 when it should return 401. (Auth errors)
  expect_equal(res$status, 401)
})

test_that("Not using username & password returns Status 401", {
  res <- httr::GET(
    "http://pecan.localhost/api/models/",
  )
  # return 200 when it should return 401. (Auth errors)
  expect_equal(res$status, 401)
})
test_that("Calling /api/inputs/{input_id} with valid parameters returns Status 200", {
  res <- httr::GET(
    paste0("http://pecan.localhost/api/inputs/", 99000000003),
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 200)
})

test_that("Calling /api/inputs/{input_id} with invalid parameters returns Status 404", {
  res <- httr::GET(
    "http://pecan.localhost/api/inputs/0",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 404)
})

test_that("Calling /api/inputs/{input_id}?filename={filename} with invalid parameters returns Status 404", {
  res <- httr::GET(
    "http://pecan.localhost/api/inputs/295?filename=random",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 400)
})
test_that("Calling /api/runs/{run_id}/graph/{year}/{yvar}/ with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/99000000282/graph/2002/GPP",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 200)
})

test_that("Calling /api/runs/{run_id}/graph/{year}/{yvar}/ with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/1000000000/graph/100/GPP",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 404)
})

test_that("Calling /api/runs/{run_id}/input/{filename} with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/99000000282/input/sipnet.in",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 200)
})

test_that("Calling /api/runs/{run_id}/input/{filename} with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/1000000000/input/randomfile",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 404)
})

test_that("Calling /api/runs/{run_id}/output/{filename} with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/99000000282/output/2002.nc",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 200)
})

test_that("Calling /api/runs/{run_id}/output/{filename} with valid inputs returns Status 200", {
  res <- httr::GET(
    "http://pecan.localhost/api/runs/1000000000/output/randomfile",
    httr::authenticate("carya", "illinois")
  )
  # No response
  expect_equal(res$status, 404)
})
# Old file : 
test_that("Calling /api/status returns Status 200", {
# SPace was present in the GET command leading to failure of this test
  res <- httr::GET(" http://pecan.localhost/api/status")
  expect_equal(res$status, 200)
})
test_that("Submitting XML workflow to /api/workflows/ returns Status 201", {
  # Utilizing absolute path for localhost docker testing instead of 'test_workflows/api.sipnet.xml'
  xml_string <- paste0(xml2::read_xml("apps/api/tests/test_workflows/api.sipnet.xml"))
  res <- httr::POST(
    "http://pecan.localhost/api/workflows/",
    httr::authenticate("carya", "illinois"),
    httr::content_type("application/xml"),
    body = xml_string
  )
  # No Response
  expect_equal(res$status, 201)
})

test_that("Submitting JSON workflow to /api/workflows/ returns Status 201", {
  Sys.sleep(2)
  json_workflow <- jsonlite::read_json("test_workflows/api.sipnet.json")
  res <- httr::POST(
    "http://pecan.localhost/api/workflows/",
    httr::authenticate("carya", "illinois"),
    body = json_workflow,
    encode='json'
  )
  # No Response
  expect_equal(res$status, 201)
})

test_that("Calling /api/workflows/{id}/file/{filename} with valid parameters returns Status 200", {
  res <- httr::GET(
    paste0("http://pecan.localhost/api/workflows/", 99000000031, "/file/", "pecan.CONFIGS.xml"),
    httr::authenticate("carya", "illinois")
  )
  # No Response
  expect_equal(res$status, 200)
})

test_that("Calling /api/workflows/{id}/file/{filename} with invalid parameters returns Status 404", {
  res <- httr::GET(
    "http://pecan.localhost/api/workflows/0/file/randomfile.txt",
    httr::authenticate("carya", "illinois")
  )
  # No Response
  expect_equal(res$status, 404)
})
infotroph commented 2 months ago

Let's plan the follow-up work in #3284 -- long comment threads in already-merged PRs can get confusing.