Feature/1693 api v3 schema etc #2052

dk1844 · 2022-04-14T14:43:10Z

Continuation of PR #2046

implements V3 API for MappingTables, Schemas, and PropDefs.
introduces Admin-level authorization for changing endpoints (POST/PUT/DELETE)
IT extended to support ⬆️ and also the test cases with incorrect auth-level (resulting with 403s)
Future work (not covered in this PR) = delete test cases (relies on assuring that the disable workflow works as expected) and used-in

Partially implements #1693

Release notes suggestion

/api-v3/{datasets|schemas|mapping-tables|property-defintions}/ Rest API V3 added. Includes checks on entities and is to be used externally. Schema is now updatable via entity payload as well (outside of attachment upload).

…e contours, empty DatasetControllerV3 to test

- login allowed at /api/login & /api-v3/login - v2/v3 BaseRestApiTest distinquished

…st cases added

…der and no content -> IT updated

… IT updated

…version, includes a sample CR)

…st and post-import + IT to prove correct behavior

…mport + IT to prove correct behavior - fix with common location processing with segment stripping (+normalization)

… GET datasets/{name}/{version}

…/audit-trail added

…ion} now works for # or 'latest' (IT = regression test)

…{name}/{version} and /{name}/latest - improved

…- supports latest for as version-expression, impl for datasets improved by actual existence checking + IT test cases for non-existing/non-latest queries

…properties - supports latest for as version-expression; get impl is unvalidated, put impl checks validity - login is now common, under /api/login for both v2 and v3 (did not work previously)

…properties - supports latest for as version-expression; get impl is unvalidated, put impl checks validity - extended for different validation cases - login is now common, under /api/login for both v2 and v3 (did not work previously)

…ture -> CompletableFuture (mistake reverted)

… added. IT mostly adjusted, but there are todos - DatasetServiceV3 introduced to carry difference in behavior to DatasetService. original entity validation has been divided into create-validation and regular-entity validation. - buildfix for VersionedModelServiceTest

…sion}` in proper validations

…asets/dsName/version/rules, GET datasets/dsName/version/rules/# + IT

- v2 ignores this added information - needs

…api-v3-schema-etc

…defaults

…ts + IT

Zejnilovic

I did not upload schema yet otherwise I think I check all the user facing paths.

...i/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/MappingTableControllerV3.scala

Zejnilovic · 2022-05-04T11:26:18Z

rest-api/src/main/scala/za/co/absa/enceladus/rest_api/services/SchemaService.scala

+    current.setDescription(update.description).asInstanceOf[Schema]
+  }
+
+  /** final - override `updateWithFields` if needed */


What is updateWithFields ? I only see updateWithFieldsReplace or updateFields

Thanks for noticing, updateFields was previously named updateWithFields and this reference was not correctly affected by the change. Fixed.

… removed, some ITs and comments added

…rt mapping removed -> reflected in IT; small updates

… removed, some ITs and comments added

…api-v3-schema-etc

rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/SchemaController.scala

benedeki · 2022-05-04T08:46:38Z

...i/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/MappingTableControllerV3.scala

+
+  import scala.concurrent.ExecutionContext.Implicits.global
+
+  @GetMapping(path = Array("/{name}/{version}/defaults"))


Sorry for my ignorance. Why we need dedicated endpoint for mapping table defaults? Isn't it enough to be part of the mapping table entity?
Maybe more a question to @Zejnilovic, as I see it in the proposal doc too.

For GET - because spark-jobs pull that data on the run of the conformance rule, in the middle of the job.
For POST - so you do not have to always update the whole thing.

Yes, the proposal contained it.

GET - 👍
PUT - Why is this different from other partial properties? (Minor, can stay 😉 )

Sort of explained in person, the gist is: a special approach is warranted due to the fact that defaults are added in now. But true that the whole endpoint may not be strictly necessary for V3 API.

Since it was designed like this, I'd like to keep it, but removal should be quite easy. 😏

benedeki · 2022-05-04T08:47:13Z

...i/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/MappingTableControllerV3.scala

+
+  @PutMapping(path = Array("/{name}/{version}/defaults"))
+  @ResponseStatus(HttpStatus.CREATED)
+  @PreAuthorize("@authConstants.hasAdminRole(authentication)")


Why the admin authorization requirement (multiple places)?

This is a mistake I plan on fixing in #2055, because there are other changes to the admin authorization that need to be done (In short: I have moved the admin-auth to VersionedModelController, but it wrong as only the PropDefs should be admin-controlled)

I have fixed this in this PR already, there will be just touchups in #2055.

benedeki · 2022-05-04T11:55:01Z

...i/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/MappingTableControllerV3.scala

+    for {
+      existingMtOpt <- forVersionExpression(name, version)(mappingTableService.getVersion)
+      existingMt = existingMtOpt.getOrElse(throw notFound())
+      updatedMtAndValidationOpt <- mappingTableService.addDefault(user.getUsername, name, existingMt.version, newDefault)
+      (updatedMt, validation) = updatedMtAndValidationOpt.getOrElse(throw notFound())
+      response = createdWithNameVersionLocationBuilder(name, updatedMt.version, request,
+        stripLastSegments = 3, suffix = s"/defaults").body(validation)  // stripping: /{name}/{version}/defaults
+    } yield response


Seems like the same code as above?

While not completely the same, there are not a lot of differences, I admit.
It did not seem like something worth generalizing to me (do you think it is?)

🤷 Up to you, sorry I missed the difference. On the other hand it suggests similarity. Counter argument is, if the code can be logically distilled not just some forced sub-function.

Ok, I have actually created a subfunction that seems to make sense. Hope you find it a tad better, @benedeki.

benedeki · 2022-05-04T11:55:54Z

...main/scala/za/co/absa/enceladus/rest_api/controllers/v3/PropertyDefinitionControllerV3.scala

+class PropertyDefinitionControllerV3 @Autowired()(propertyDefService: PropertyDefinitionService)
+  extends VersionedModelControllerV3(propertyDefService)
+
+// super-class implementation is sufficient


Appreciate the comment 😎

Glad you liked it, but I actually had to redo it

benedeki · 2022-05-04T12:02:57Z

rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/SchemaControllerV3.scala

+import za.co.absa.enceladus.rest_api.models.rest.exceptions.SchemaParsingException
+import za.co.absa.enceladus.rest_api.repositories.RefCollection
+import za.co.absa.enceladus.rest_api.services.v3.SchemaServiceV3
+import za.co.absa.enceladus.rest_api.services.{AttachmentService, SchemaRegistryService, SchemaService}


SchemaService is not used

True, thanks, removed.

rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/SchemaControllerV3.scala

rest-api/src/main/scala/za/co/absa/enceladus/rest_api/services/v3/MappingTableServiceV3.scala

…ing,

…authentication)")` is now limited to changing endpoints of PropertyDefinitionControllerV3 (previously it was incorrectly used for all changing endpoints. IT updated. And specific check for the adminRole have been added to PropertyDefinitionControllerV3IntegrationSuite.

…g `dev` or not.

…api-v3-schema-etc

benedeki

Just read the code. Comments are not blocking.

rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/SchemaController.scala

benedeki · 2022-05-09T08:42:31Z

...i/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/MappingTableControllerV3.scala

+
+  import scala.concurrent.ExecutionContext.Implicits.global
+
+  @GetMapping(path = Array("/{name}/{version}/defaults"))


GET - 👍
PUT - Why is this different from other partial properties? (Minor, can stay 😉 )

benedeki · 2022-05-09T08:44:29Z

...i/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/MappingTableControllerV3.scala

+    for {
+      existingMtOpt <- forVersionExpression(name, version)(mappingTableService.getVersion)
+      existingMt = existingMtOpt.getOrElse(throw notFound())
+      updatedMtAndValidationOpt <- mappingTableService.addDefault(user.getUsername, name, existingMt.version, newDefault)
+      (updatedMt, validation) = updatedMtAndValidationOpt.getOrElse(throw notFound())
+      response = createdWithNameVersionLocationBuilder(name, updatedMt.version, request,
+        stripLastSegments = 3, suffix = s"/defaults").body(validation)  // stripping: /{name}/{version}/defaults
+    } yield response


🤷 Up to you, sorry I missed the difference. On the other hand it suggests similarity. Counter argument is, if the code can be logically distilled not just some forced sub-function.

… with `MappingTableControllerV3.withMappingTableToResponse`

% Conflicts: % rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/SchemaController.scala % rest-api/src/main/scala/za/co/absa/enceladus/rest_api/controllers/v3/VersionedModelControllerV3.scala % rest-api/src/main/scala/za/co/absa/enceladus/rest_api/services/SchemaService.scala % rest-api/src/main/scala/za/co/absa/enceladus/rest_api/services/VersionedModelService.scala % rest-api/src/test/scala/za/co/absa/enceladus/rest_api/integration/controllers/v3/DatasetControllerV3IntegrationSuite.scala

sonarcloud · 2022-05-09T11:20:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
24 Code Smells

No Coverage information
2.6% Duplication

Zejnilovic

Tested the endpoints.
Code reviewed

AdrianOlosutean

Looks good

dk1844 added 30 commits March 7, 2022 15:35

#1693 API v3: VersionedModel v3 init - VersionedModelControllerV3 bas…

f1798b2

…e contours, empty DatasetControllerV3 to test

#1693 API v3: some basic VersionedModelControllerV3 integration test

0f62851

- login allowed at /api/login & /api-v3/login - v2/v3 BaseRestApiTest distinquished

#1693 API v3: VersionedModelControllerV3 integration test cont'd - te…

c01cfda

…st cases added

#1693 API v3: VersionedModelControllerV3 - post to yield location hea…

4fd08e2

…der and no content -> IT updated

#1693 API v3: VersionedModelControllerV3 - put to yield no content ->…

5c8e005

… IT updated

#1693 API v3: VersionedModelControllerV3 - IT for /ds/export (latest …

458bfed

…version, includes a sample CR)

#1693 API v3: VersionedModelControllerV3 - location header for put/po…

a7a6cf9

…st and post-import + IT to prove correct behavior

#1693 API v3: VersionedModelControllerV3 - location header for post-i…

af489b1

…mport + IT to prove correct behavior - fix with common location processing with segment stripping (+normalization)

#1693 API v3: VersionedModelControllerV3 -IT for GET datasets/{name},…

21f8008

… GET datasets/{name}/{version}

#1693 API v3: VersionedModelControllerV3 - IT for GET datasets/{name}…

6f63117

…/audit-trail added

#1693 API v3: VersionedModelControllerV3 - GET /datasets/{name}/{vers…

76e87ce

…ion} now works for # or 'latest' (IT = regression test)

#1693 API v3: VersionedModelControllerV3 - common implementation of /…

d62cd90

…{name}/{version} and /{name}/latest - improved

#1693 API v3: VersionedModelControllerV3 - /{name}/{version}/used-in …

fc4c359

…- supports latest for as version-expression, impl for datasets improved by actual existence checking + IT test cases for non-existing/non-latest queries

#1693 API v3: VersionedModelControllerV3 - GET/PUT /{name}/{version}/…

e6c8d8c

…properties - supports latest for as version-expression; get impl is unvalidated, put impl checks validity - login is now common, under /api/login for both v2 and v3 (did not work previously)

#1693 Fix: (v2) VersionedModelController.getLatestVersionNumber(): Fu…

219360a

…ture -> CompletableFuture (mistake reverted)

#1693 VersionedModelControllerV3 - IT extended for `POST /{name}/{ver…

101b37a

…sion}` in proper validations

#1693 VersionedModelControllerV3 - conformance rule mgmt GET+POST dat…

b934958

…asets/dsName/version/rules, GET datasets/dsName/version/rules/# + IT

#1693 PR review update - typos etc

51ad013

#1693 Validation-warnings as payload to caller in V3

ba5edce

- v2 ignores this added information - needs

#1693 Validation-warnings as payload to caller in V3 - IT covered

662119b

#1693 API v3 impl for MTs, no tests yet

b0fd955

#1693 import validation check added to IT.

d480445

Merge branch 'feature/1693-api-v3-versioned-model' into feature/1693-…

bffbf55

…api-v3-schema-etc

Merge branch 'develop-ver-3.0' into feature/1693-api-v3-versioned-model

3542a43

Merge branch 'feature/1693-api-v3-versioned-model' into feature/1693-…

51d68a8

…api-v3-schema-etc

#1693 api-v3 mapping table specific - IntegTest for GET/POST/PUT .../…

e1c904e

…defaults

#1693 api-v3 mapping table specific - MT now validates if schema exis…

489ae12

…ts + IT

#1693 schema validation for DSs + IntegTest update

d6ef401

Zejnilovic reviewed May 4, 2022

View reviewed changes

dk1844 added 6 commits May 4, 2022 14:45

#1693 VersionedMongoRepository - unused method getAllVersionsValues…

56a9f0c

… removed, some ITs and comments added

#1693 for V3 API: VersionedSummary -> NamedLatestVersion: impl + IT

a4aac70

#1693 PR review update: VersionedModelControllerV3 - GET /{name}/expo…

5d24819

…rt mapping removed -> reflected in IT; small updates

#1693 VersionedMongoRepository - unused method getAllVersionsValues…

7040694

… removed, some ITs and comments added

#1693 for V3 API: VersionedSummary -> NamedLatestVersion: impl + IT

e0fda73

Merge branch 'feature/1693-api-v3-versioned-model' into feature/1693-…

2bfd751

…api-v3-schema-etc

benedeki reviewed May 4, 2022

View reviewed changes

dk1844 added 4 commits May 5, 2022 12:39

#1693 PR review updates: renamings, comments, removed unused, formatt…

1da8e8c

…ing,

#1693 Swagger differs in paths based on spring.profiles.active bein…

a80d9ac

…g `dev` or not.

Merge branch 'feature/1693-api-v3-versioned-model' into feature/1693-…

f851bce

…api-v3-schema-etc

dk1844 requested a review from benedeki May 6, 2022 12:22

dk1844 added 2 commits May 9, 2022 09:21

#1693 Swagger API: dev-profile: full v2+v3 API, non-dev: full v3 API

562da60

Merge branch 'feature/1693-api-v3-versioned-model' into feature/1693-…

fb0dd93

…api-v3-schema-etc

dk1844 requested a review from Zejnilovic May 9, 2022 08:13

benedeki previously approved these changes May 9, 2022

View reviewed changes

Base automatically changed from feature/1693-api-v3-versioned-model to develop-ver-3.0 May 9, 2022 08:49

#1693 PRReview /{name}/{version}/defaults processing generalization…

56ba9ed

… with `MappingTableControllerV3.withMappingTableToResponse`

dk1844 dismissed benedeki’s stale review via 56ba9ed May 9, 2022 10:37

Zejnilovic approved these changes May 10, 2022

View reviewed changes

Zejnilovic added the PR:tested Only for PR - PR was tested by a tester (person) label May 10, 2022

AdrianOlosutean approved these changes May 10, 2022

View reviewed changes

benedeki approved these changes May 10, 2022

View reviewed changes

dk1844 merged commit 0576893 into develop-ver-3.0 May 10, 2022

dk1844 deleted the feature/1693-api-v3-schema-etc branch May 10, 2022 09:28

dk1844 linked an issue Jun 7, 2022 that may be closed by this pull request

Experimental API 3 - Versioned Model #1693

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/1693 api v3 schema etc #2052

Feature/1693 api v3 schema etc #2052

dk1844 commented Apr 14, 2022 •

edited

Loading

Zejnilovic left a comment

Zejnilovic May 4, 2022

dk1844 May 5, 2022

benedeki May 4, 2022

Zejnilovic May 5, 2022

dk1844 May 5, 2022

benedeki May 9, 2022

dk1844 May 9, 2022

benedeki May 4, 2022

dk1844 May 5, 2022 •

edited

Loading

benedeki May 4, 2022

dk1844 May 5, 2022 •

edited

Loading

benedeki May 9, 2022

dk1844 May 9, 2022

benedeki May 4, 2022

dk1844 May 6, 2022

benedeki May 4, 2022

dk1844 May 5, 2022

benedeki left a comment

benedeki May 9, 2022

benedeki May 9, 2022

sonarcloud bot commented May 9, 2022

Zejnilovic left a comment

AdrianOlosutean left a comment


		import scala.concurrent.ExecutionContext.Implicits.global

		@GetMapping(path = Array("/{name}/{version}/defaults"))

Feature/1693 api v3 schema etc #2052

Feature/1693 api v3 schema etc #2052

Conversation

dk1844 commented Apr 14, 2022 • edited Loading

Release notes suggestion

Zejnilovic left a 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

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

dk1844 May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dk1844 May 5, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benedeki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 9, 2022

Zejnilovic left a comment

Choose a reason for hiding this comment

AdrianOlosutean left a comment

Choose a reason for hiding this comment

dk1844 commented Apr 14, 2022 •

edited

Loading

dk1844 May 5, 2022 •

edited

Loading

dk1844 May 5, 2022 •

edited

Loading