feat: Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route #2355

seakayone · 2022-12-27T10:49:18Z

Pull Request Checklist

Task Description/Number

Picking up from the last mob/main session I have finalised the changes from that branch.

Changes include:

Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route.
Remove unused requestingUser field from ProjectGetRequestADM.
Add AuthenticatorService - a zio service for interaction with Authenticator as a preparation for other routes.

Basic Requirements

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix: represents bug fixes
Refactor: represents production code refactoring
Feature: represents a new feature
Documentation: documentation changes (no production code change)
Chore: maintenance tasks (no production code change)
Style: styles updates (no production code change)
Test: all about tests: adding, refactoring tests (no production code change)
Other... Please describe:

Does this PR introduce a breaking change?

Yes
No

Does this PR change client-test-data?

Yes (don't forget to update the JS-LIB team about the change)
No

lastFile:webapi/src/main/scala/org/knora/webapi/core/HttpServerWithZIOHttp.scala

…Spec pass

codecov · 2022-12-27T10:55:45Z

Codecov Report

Base: 86.68% // Head: 86.45% // Decreases project coverage by -0.22% ⚠️

Coverage data is based on head (aaf8429) compared to base (26e9596).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2355      +/-   ##
==========================================
- Coverage   86.68%   86.45%   -0.23%     
==========================================
  Files         250      271      +21     
  Lines       28252    28270      +18     
==========================================
- Hits        24490    24441      -49     
- Misses       3762     3829      +67

Impacted Files	Coverage Δ
...p-shared/src/main/scala/dsp/valueobjects/Iri.scala	`93.93% <100.00%> (+0.12%)`	⬆️
...sp-shared/src/main/scala/dsp/valueobjects/V2.scala	`90.27% <100.00%> (+0.27%)`	⬆️
.../scala/org/knora/webapi/responders/Responder.scala	`75.00% <0.00%> (-16.43%)`	⬇️
...org/knora/webapi/routing/v2/ResourcesRouteV2.scala	`88.27% <0.00%> (-5.79%)`	⬇️
.../webapi/messages/util/rdf/jenaimpl/JenaModel.scala	`86.42% <0.00%> (-2.86%)`	⬇️
...n/scala/org/knora/webapi/routing/RouteUtilV2.scala	`64.93% <0.00%> (-1.74%)`	⬇️
...la/org/knora/webapi/messages/StringFormatter.scala	`88.72% <0.00%> (-0.80%)`	⬇️
...api/responders/admin/PermissionsResponderADM.scala	`90.16% <0.00%> (-0.04%)`	⬇️
...knora/webapi/responders/v1/ValuesResponderV1.scala	`92.73% <0.00%> (-0.01%)`	⬇️
...ra/webapi/responders/v2/ResourcesResponderV2.scala	`93.43% <0.00%> (-0.01%)`	⬇️
... and 65 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…alculateCookieName

…, and remove new needless implicit class tag

…ore generic.

…e.getSingleProjectADMRequest

BalduinLandolt

Very nice to have the authenticator separated out like this!

Some questions/remarks:

Why did you rename ProjectsService to RestProjectsService? What are our naming conventions in this regard?
Maybe I'm overlooking something... but I think the addition of HealthRouteZ is not reflected in the PR title/description.
Otherwise, some comments in the files directly.

webapi/src/main/scala/org/knora/webapi/http/status/ApiStatusCodesZ.scala

webapi/src/main/scala/org/knora/webapi/responders/admin/ProjectsResponderADM.scala

webapi/src/main/scala/org/knora/webapi/responders/admin/RestProjectsService.scala

webapi/src/main/scala/org/knora/webapi/routing/HealthRouteZ.scala

webapi/src/main/scala/org/knora/webapi/routing/RouteUtilZ.scala

webapi/src/main/scala/org/knora/webapi/routing/admin/ProjectsRouteZ.scala

BalduinLandolt · 2023-01-02T08:53:25Z

webapi/src/main/scala/org/knora/webapi/routing/admin/ProjectsRouteZ.scala

-  val layer: URLayer[ProjectsService with AppConfig, ProjectsRouteZ] = ZLayer.fromFunction(ProjectsRouteZ.apply _)
+  val layer: ZLayer[AppConfig with RestProjectsService, Nothing, ProjectsRouteZ] =
+    ZLayer.fromFunction(ProjectsRouteZ.apply _)


Why not keep URLayer?

mpro7

Feel free to apply my suggestions - mainly Docstrings improvements.

mpro7 · 2023-01-02T11:16:05Z

webapi/src/main/scala/org/knora/webapi/http/status/ApiStatusCodesZ.scala

 * The possible values for the HTTP status code that is returned as part of each Knora API v2 response.
+ * migrated from [[org.knora.webapi.http.status.ApiStatusCodes]]


Suggested change

* migrated from [[org.knora.webapi.http.status.ApiStatusCodes]]

* Migrated from [[org.knora.webapi.http.status.ApiStatusCodes]]

webapi/src/main/scala/org/knora/webapi/routing/HealthRouteZ.scala

mpro7 · 2023-01-02T12:13:41Z

webapi/src/main/scala/org/knora/webapi/routing/HealthRouteZ.scala

+   * @param name     ???
+   * @param severity ???


irinaschubert

LGTM, just some minor remarks

webapi/src/main/scala/org/knora/webapi/routing/RouteUtilZ.scala

irinaschubert · 2023-01-02T12:10:16Z

webapi/src/main/scala/org/knora/webapi/routing/RouteUtilZ.scala

+   *
+   *         ```failure``` A [[BadRequestException]] with the `errorMsg`
+   */
+  def urlDecode(value: String, errorMsg: String = ""): Task[String] =


Do we actually need the second parameter errorMsg?

Yes, different Routes might want create their own error messages in the BadRequestException. In this PR it is "Failed to url decode IRI parameter." in the route. I did not want to hardcode that particular message into the RouteUtil as it should not assume that all encoded params are IRIs.

irinaschubert · 2023-01-02T12:11:35Z

webapi/src/main/scala/org/knora/webapi/routing/admin/ProjectsRouteZ.scala

+        // Returns a single project identified by an urlencoded IRI
+        case Method.GET -> !! / "admin" / "projects" / "iri" / iriUrlEncoded =>
+          RouteUtilZ
+            .urlDecode(iriUrlEncoded, "Failed to url decode IRI parameter.")


Suggested change

.urlDecode(iriUrlEncoded, "Failed to url decode IRI parameter.")

.urlDecode(iriUrlEncoded, s"Failed to URL decode IRI from parameter: ${iriUrlEncoded}.")

irinaschubert · 2023-01-02T12:13:16Z

webapi/src/test/scala/org/knora/webapi/responders/admin/RestProjectsServiceSpec.scala

    .of[ProjectGetResponseADM]
    .apply(Assertion.equalTo(expectedRequest), Expectation.value(expectedResponse))

+  private val expectNoInteraction = ActorToZioBridgeMock.empty
+  private val systemUnderTest     = ZIO.service[RestProjectsService]


Suggested change

private val systemUnderTest = ZIO.service[RestProjectsService]

private val serviceUnderTest = ZIO.service[RestProjectsService]

webapi/src/test/scala/org/knora/webapi/routing/RouteUtilZSpec.scala

irinaschubert · 2023-01-02T12:14:39Z

webapi/src/test/scala/org/knora/webapi/routing/RouteUtilZSpec.scala

+          actual <- RouteUtilZ.urlDecode("http%3A%2F%2Frdfh.ch%2Fprojects%2FLw3FC39BSzCwvmdOaTyLqQ")
+        } yield assertTrue(actual == "http://rdfh.ch/projects/Lw3FC39BSzCwvmdOaTyLqQ")
+      },
+      test("given an empty value should return BadRequestException") {


Suggested change

test("given an empty value should return BadRequestException") {

test("given an empty valu, return BadRequestException") {

webapi/src/test/scala/org/knora/webapi/routing/RouteUtilZSpec.scala

irinaschubert · 2023-01-02T12:16:54Z

webapi/src/main/scala/org/knora/webapi/responders/admin/RestProjectsService.scala

+final case class RestProjectsService(bridge: ActorToZioBridge) {
+
+  /**
+   * Finds the project by its [[ProjectIdentifierADM]] and returns the information as a [[ProjectGetResponseADM]].


Suggested change

* Finds the project by its [[ProjectIdentifierADM]] and returns the information as a [[ProjectGetResponseADM]].

* Finds the project by its [[IRI]] and returns the information as a [[ProjectGetResponseADM]].

…desZ.scala Co-authored-by: Balduin Landolt <33053745+BalduinLandolt@users.noreply.github.com>

Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>

Co-authored-by: Marcin Procyk <marcin.procyk@dasch.swiss>

…scala Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>

seakayone added 6 commits December 24, 2022 00:07

mob next [ci-skip] [ci skip] [skip ci]

480b06d

lastFile:webapi/src/main/scala/org/knora/webapi/core/HttpServerWithZIOHttp.scala

add test

3abdc21

Remove unused user param from get single project, make ProjectsRouteZ…

a163b90

…Spec pass

fmt

d568e43

Rename RestProjectsService and add test

b6fedf6

fmt

d8eb0c1

seakayone added 2 commits December 27, 2022 12:23

clean up code: make credential extraction lazy, reuse Authenticator.c…

2935a6c

…alculateCookieName

add test for AuthenticatorServiceLive

0f0c5f6

seakayone force-pushed the wip/projectroute-zio branch 2 times, most recently from b16f9cc to 5600a18 Compare December 27, 2022 14:35

add query param extraction and tests for AuthenticatorServiceLive

ba8ca55

seakayone force-pushed the wip/projectroute-zio branch from 5600a18 to ba8ca55 Compare December 27, 2022 14:43

seakayone added 5 commits December 27, 2022 16:44

fix ActorToZioBridge by explicitly mapping in right execution context…

df79afc

…, and remove new needless implicit class tag

Move ProjectsRouteZSpec to test source set and add types

8703711

fmt

922d2cc

fmt

e6d838b

fmt

ad5b367

seakayone force-pushed the wip/projectroute-zio branch from 55a5348 to ad5b367 Compare December 28, 2022 10:12

seakayone changed the title ~~Wip/projectroute zio~~ feature: Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route Dec 28, 2022

seakayone added 2 commits December 28, 2022 12:04

Rename RouteUtilZ.urlDecode function and make default error message m…

39650f8

…ore generic.

fix scaladoc for org.knora.webapi.responders.admin.RestProjectsServic…

4ca3b82

…e.getSingleProjectADMRequest

seakayone marked this pull request as ready for review December 28, 2022 11:21

seakayone requested review from mpro7, irinaschubert and BalduinLandolt December 28, 2022 11:21

Remove unused lookups to requestingUser from ProjectsRouteADM

c5abd17

seakayone self-assigned this Jan 2, 2023

BalduinLandolt approved these changes Jan 2, 2023

View reviewed changes

mpro7 approved these changes Jan 2, 2023

View reviewed changes

irinaschubert approved these changes Jan 2, 2023

View reviewed changes

mpro7 changed the title ~~feature: Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route~~ feat: Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route Jan 2, 2023

seakayone and others added 13 commits January 2, 2023 16:30

Update webapi/src/main/scala/org/knora/webapi/http/status/ApiStatusCo…

20c47ce

…desZ.scala Co-authored-by: Balduin Landolt <33053745+BalduinLandolt@users.noreply.github.com>

fixup

12100ca

extract method

70d6cf6

use urlayer

3e22ae7

Update webapi/src/main/scala/org/knora/webapi/routing/RouteUtilZ.scala

5ee81e1

Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>

scaladoc

e8529a0

Update webapi/src/main/scala/org/knora/webapi/routing/HealthRouteZ.scala

8407429

Co-authored-by: Marcin Procyk <marcin.procyk@dasch.swiss>

Update webapi/src/main/scala/org/knora/webapi/routing/HealthRouteZ.scala

7885224

Co-authored-by: Marcin Procyk <marcin.procyk@dasch.swiss>

Update webapi/src/main/scala/org/knora/webapi/routing/HealthRouteZ.scala

1b81714

Co-authored-by: Marcin Procyk <marcin.procyk@dasch.swiss>

Update webapi/src/test/scala/org/knora/webapi/routing/RouteUtilZSpec.…

cbe004e

…scala Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>

Update webapi/src/test/scala/org/knora/webapi/routing/RouteUtilZSpec.…

c2313b5

…scala Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>

Merge branch 'main' into wip/projectroute-zio

7a33a0d

fmt

aaf8429

seakayone merged commit 2f42906 into main Jan 2, 2023

seakayone deleted the wip/projectroute-zio branch January 2, 2023 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route #2355

feat: Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route #2355

seakayone commented Dec 27, 2022 •

edited

codecov bot commented Dec 27, 2022 •

edited

BalduinLandolt left a comment

BalduinLandolt Jan 2, 2023

mpro7 left a comment

mpro7 Jan 2, 2023

mpro7 Jan 2, 2023

irinaschubert left a comment

irinaschubert Jan 2, 2023

seakayone Jan 2, 2023 •

edited

irinaschubert Jan 2, 2023

irinaschubert Jan 2, 2023

irinaschubert Jan 2, 2023

irinaschubert Jan 2, 2023

		* The possible values for the HTTP status code that is returned as part of each Knora API v2 response.
		* migrated from [[org.knora.webapi.http.status.ApiStatusCodes]]

	.urlDecode(iriUrlEncoded, "Failed to url decode IRI parameter.")
	.urlDecode(iriUrlEncoded, s"Failed to URL decode IRI from parameter: ${iriUrlEncoded}.")

	private val systemUnderTest = ZIO.service[RestProjectsService]
	private val serviceUnderTest = ZIO.service[RestProjectsService]

	test("given an empty value should return BadRequestException") {
	test("given an empty valu, return BadRequestException") {

	* Finds the project by its [[ProjectIdentifierADM]] and returns the information as a [[ProjectGetResponseADM]].
	* Finds the project by its [[IRI]] and returns the information as a [[ProjectGetResponseADM]].

feat: Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route #2355

feat: Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route #2355

Conversation

seakayone commented Dec 27, 2022 • edited

Pull Request Checklist

Task Description/Number

Basic Requirements

PR Type

Does this PR introduce a breaking change?

Does this PR change client-test-data?

codecov bot commented Dec 27, 2022 • edited

Codecov Report

BalduinLandolt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpro7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irinaschubert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seakayone Jan 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seakayone commented Dec 27, 2022 •

edited

codecov bot commented Dec 27, 2022 •

edited

seakayone Jan 2, 2023 •

edited