feat: Expose GET /admin/projects/iri/{iriUrlEncoded} as zio-http route by seakayone · Pull Request #2355 · dasch-swiss/dsp-api

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.

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

-  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

- * Migrated from [[org.knora.webapi.http.status.ApiStatusCodes]]
- *
 * 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]]

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

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


irinaschubert

LGTM, just some minor remarks

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

+   *
+   *         ```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

+        // 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

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

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

+          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") {

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

+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>

Christian Kleinbölting 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

Christian Kleinbölting 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

Christian Kleinbölting 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

Christian Kleinbölting 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 BalduinLandolt, irinaschubert and mpro7 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

	* migrated from [[org.knora.webapi.http.status.ApiStatusCodes]]
	* 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]].

Conversation

seakayone commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Task Description/Number

Basic Requirements

PR Type

Does this PR introduce a breaking change?

Does this PR change client-test-data?

Uh oh!

codecov Bot commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

BalduinLandolt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpro7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

irinaschubert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seakayone Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seakayone commented Dec 27, 2022 •

edited

Loading

codecov Bot commented Dec 27, 2022 •

edited

Loading

seakayone Jan 2, 2023 •

edited

Loading