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

Update API endpoint URLs #3283

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Conversation

Sweetdevil144
Copy link
Contributor

This Fix is similar to this issue in our rpecanapi Repository : PecanProject/rpecanapi#21

Description

Refracted server URLs to use localhost PEcAn URLs

Motivation and Context

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@github-actions github-actions bot added the Tests label Apr 4, 2024
@mdietze mdietze added this pull request to the merge queue Apr 4, 2024
Merged via the queue into PecanProject:develop with commit 34b103c Apr 4, 2024
13 checks passed
@infotroph
Copy link
Member

@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
Copy link
Contributor Author

Sweetdevil144 commented Apr 5, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Conclusion

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

  • For many of our test cases within apps/api/tests/ folder, pecan.localhost works the same as localhost:8000, only locally however. Can you confirm whether this is true again @infotroph ?
  • Some few selected files return either 400, 401, 404 or do not GET at all and take infinite time causing system barriers to exist.

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

  • apps/api/tests/test.auth.R : 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")
  )
  # 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)
})
  • apps/api/tests/test.inputs.R : Following test cases
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)
})
  • apps/api/tests/test.runs.R : Following test cases
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)
})
  • apps/api/tests/test.status.R : Following test cases
# 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)
})
  • apps/api/tests/test.workflows.R : Following test cases
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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants