Feature/1693 api v3 versioned model - Dataset v3 implementation #2046

dk1844 · 2022-04-04T14:00:51Z

This PR currently implements Enceladus API v3 for

general versioned model v3
- note that general /{modelName}/{entityName}/{versions}/validate exists
dataset model v3; for the dataset API, a few changes have been done in the design:
- .../properties sub-endpoints has been moved from the general versioned model to datasets (dataset-specific feature)
- .../properties/{propertyName} has not been implemented, since the .../properties endpoint has no additional logic of validation (because of the existence of a general validation endpoint instead)
later ADDED: schema and MT existence checking (those referenced in datasets (for MTs - in MCR))
schemas, mapping tables, and property definitions are to follow; probably in another PR.

Partially implements #1693
Closes #1611

Release notes suggestion

/api-v3/datasets/ Rest API V3 added. Includes checks on entities and is to be used externally. The API is now truly RESTful (endpoint naming, structure, methods, Location header on creating responses). Also, Swagger API doc reacts to Spring dev profile (JVM arg -Dspring.profiles.active=dev) to show legacy V2 API doc, too.

…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

AdrianOlosutean

I like it overall, just some small comments.

Also, do you plan to change the frontend to use this api?

data-model/src/main/scala/za/co/absa/enceladus/model/versionedModel/VersionedSummary.scala

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

AdrianOlosutean · 2022-04-05T10:14:59Z

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

+  @GetMapping(Array("/{name}/export"))
+  @ResponseStatus(HttpStatus.OK)
+  def exportLatestEntity(@PathVariable name: String): CompletableFuture[String] = {
+    versionedModelService.exportLatestItem(name) // todo: remove in favor of the above? (that supports /{name}/latest/export)


From my point of view, this can stay

Well, this is part of the v3 API outline. I consider this misleading, because since you have the option to export specific versions already ( at .../{model}/{name}/{versionEitherNumberOrLatestKeyword}/export, having the option for export here would have to have another meaning - e.g. export the whole entity with all the versions. But since this is designed as the latest export as well, I consider this unnecessary, and as I said, even misleading.

That is why I am proposing to omit this endpoint entirely.

I think that if we gave the "latest" version we can omit this.

Another point to think about can be - export/import being on the entity itself. Export having parameters of name and version.

After even further discussion, I have decided to remove the endpoint based on the following logic:

The structure of API endpoints representing a collection should, in general, behave in the following fashion:

GET /entityNameInPlural -> list items or provide information about all/more items

GET /entityNameInPlural/individualEntityKeyEgId -> give information about a specific entity

By that logic, GET /{name}/export would be appropriate if all entities or information about all/more was returned -- perhaps all versions, while GET /{name}/{version}/export would export just a single version.

But since we mapped just the single version (the latest one) export to a collection's endpoint, the logic does not fit. Moreover, given the fact that we also accept latest as a valid version identifier, it seem much more appropriate to use GET /{name}/{version}/export here (specifically GET /{name}/latest/export)

dk1844 · 2022-04-06T10:53:48Z

...
Also, do you plan to change the frontend to use this API?

In the end, I think the UI should get changed to use this API. But that is obviously outside of the scope of the API update PRs.

- v2 ignores this added information - needs

benedeki · 2022-04-21T08:58:48Z

data-model/src/main/scala/za/co/absa/enceladus/model/versionedModel/VersionedSummary.scala

@@ -16,3 +16,5 @@
 package za.co.absa.enceladus.model.versionedModel

 case class VersionedSummary(_id: String, latestVersion: Int)
+
+case class VersionList(_id: String, versions: Seq[Int])


Couldn't Range be used for versions?

In theory, it could. Or even VersionedSummary with just the latest version would do (assuming that all entities start with v1).

Attachments are the exception, they take after the schema version and there may be holes, but attachments are not exposed directly.

Then there is the matter of not being able to prove that all versions are actually there in the DB, we assume they should be, but... how sure can you be.

Perhaps let's just use just VersionedSummary - with the latest version

data-model/src/main/scala/za/co/absa/enceladus/model/versionedModel/VersionedSummary.scala

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

benedeki · 2022-04-26T08:10:06Z

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

@@ -113,7 +113,7 @@ class DatasetController @Autowired()(datasetService: DatasetService)
  def replaceProperties(@AuthenticationPrincipal principal: UserDetails,
                        @PathVariable datasetName: String,
                        @RequestBody newProperties: Optional[Map[String, String]]): CompletableFuture[ResponseEntity[Option[Dataset]]] = {
-    datasetService.replaceProperties(principal.getUsername, datasetName, newProperties.toScalaOption).map {
+    datasetService.updatePropertiesV2(principal.getUsername, datasetName, newProperties.toScalaOption).map {


I guess the V2 and V3 suffixes are because of Spring autowiring?
(Looks ugly to me, to be honest 😄 )

No, this has nothing to do with spring autowiring. This is just a way to have V2 and V3 implementation in some services so that the v2 version can be removed easily when only v3 is to stay.

If we want to keep v2 and v3 long-term, this would require to create separate services, e.g. datasetService and datasetServiceV3. One of these solutions is, I believe, necessary if you want to keep the original implementation working.

It then begs the question - what is the intended lifetime of V2 API? Knowing this better, it could help to design better: copy-paste-and-change-a-few-things knowing that the V2 impl will be gone soon vs. keeping v3 extending v2 (no code duplication, but harder removal)

The v3 method moved to DatasetServiceV3, so now it is tidier. Thanks for the tip, @benedeki.

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

benedeki · 2022-04-26T09:29:12Z

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

+      validation <- for {
+        generalValidation <- validate(item)
+        creationValidation <- validateForCreation(item)
+      } yield generalValidation.merge(creationValidation)


This will execute the Futures in serial manner.

Suggested change

validation <- for {

generalValidation <- validate(item)

creationValidation <- validateForCreation(item)

} yield generalValidation.merge(creationValidation)

val generalValidationFuture = validate(item)

val creationValidationFuture = validateForCreation(item)

validation <- for {

generalValidation <- generalValidationFuture

creationValidation <- creationValidationFuture

} yield generalValidation.merge(creationValidation)

Yes, I don't see it as a problem (faster multiple-at-once vs possibly not needed - I don't even see a clear winner here)

TODO Let's comment in place on how the for-comprehension will only run the necessary checks up until the point where it fails first.

Explanatory comment added, thanks for the tip.

benedeki · 2022-04-26T12:44:30Z

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

+    for {
+      originalValidation <- super.validate(item)
+      propertiesValidation <- validateProperties(item.propertiesAsMap)
+      schemaValidation <- validateSchemaExists(item.schemaName, item.schemaVersion)
+      rulesValidation <- validateRules(item)


Ditto with concurrency:

Suggested change

for {

originalValidation <- super.validate(item)

propertiesValidation <- validateProperties(item.propertiesAsMap)

schemaValidation <- validateSchemaExists(item.schemaName, item.schemaVersion)

rulesValidation <- validateRules(item)

val originalValidationFuture = super.validate(item)

val propertiesValidationFuture = validateProperties(item.propertiesAsMap)

val schemaValidationFuture = validateSchemaExists(item.schemaName, item.schemaVersion)

val rulesValidationFuture = validateRules(item)

for {

originalValidation <- originalValidationFuture

propertiesValidation <- propertiesValidationFuture

schemaValidation <- schemaValidationFuture

rulesValidation <- rulesValidationFuture

same as above.

Explanatory comment added, thanks for the tip.

AdrianOlosutean

Looks good to me

Zejnilovic · 2022-05-03T08:50:19Z

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

+    or[String](
+      // api v2
+      regex("/api/dataset.*"), regex("/api/schema.*"),
      regex("/api/mappingTable.*"), regex("/api/properties.*"),
-      regex("/api/monitoring.*"),regex("/api/runs.*"),
+      regex("/api/monitoring.*"), regex("/api/runs.*"),
      regex("/api/user.*"), regex("/api/spark.*"),
-      regex("/api/configuration.*")
+      regex("/api/configuration.*"),


I would not document v2 API as we do not want people to use it. Thoughts?

Maybe spring fox could have multiple profiles? Dev and non-dev

I agree with not documenting v2

So, I have changed the implementation to the following:

JVM being supplied -Dspring.profiles.active=dev will result in full V2 + V3 API as before,

by default (no special profile setup) will only allow Swagger to show API V3 + import/export methods of API v2 (these we consider public API in V2 already).

Hope this is satisfactory.

Showcase DEV -Dspring.profiles.active=dev

Showcase Non-DEV

Redone due to #2046 (comment).
Menas DEV API remains unchanged, for Menas non-DEV API now only contains V3 API endpoints (i.e. V2 import/export removed)

…vice and DatasetServiceV3 with renaming. VersionList removed in favor of VersionedSummary everywhere. - explanatory comments

Zejnilovic

I think I have tested all the endpoints at this point.

Have not reviewed all the code.

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

Zejnilovic · 2022-05-03T11:48:10Z

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

+    versionedModelService.getAuditTrail(name)
+  }
+
+  @GetMapping(Array("/{name}/{version}/used-in"))


Does a global used-in make sense since Dataset does not need it? I think this should be specific for the entity.

Here, I think it makes sense to keep it.

Since the disable is available for all entity types, so should used-in. The nice thing about it is that there can be a common approach to entities:

making sure that there is anything preventing from disabling (used-in)

dealing with linked references returned by used-in if any

disabling the entity knowing that it should succeed (without surprises)

It is quite reassuring for the API user to be able to call used-in even if currently for a certain type there cannot be any entity linked, it perhaps may be in the future?

All in all, I am a fan of having used-ins everywhere (where disable is).

Zejnilovic · 2022-05-03T12:27:38Z

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

+  @GetMapping(Array("/{name}/export"))
+  @ResponseStatus(HttpStatus.OK)
+  def exportLatestEntity(@PathVariable name: String): CompletableFuture[String] = {
+    versionedModelService.exportLatestItem(name) // todo: remove in favor of the above? (that supports /{name}/latest/export)


I think that if we gave the "latest" version we can omit this.

Another point to think about can be - export/import being on the entity itself. Export having parameters of name and version.

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

data-model/src/main/scala/za/co/absa/enceladus/model/versionedModel/VersionedSummary.scala

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

… removed, some ITs and comments added

…g `dev` or not.

Zejnilovic · 2022-05-09T07:12:25Z

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

+    val paths: Seq[Predicate[String]] = if (isDev) {
+      v2devPaths ++ v3paths
+    } else {
+      v2prodPaths ++ v3paths


Suggested change

v2prodPaths ++ v3paths

v3paths

So prod APIs should be only v3 paths as the import-export being "prod" is only an ABSA thing.

This would result in dev having all v2 and v3 paths and prod only v3.

Thanks for the clarification, suggestion applied (along with removing the unnecessary code)

sonarcloud · 2022-05-09T07:29:59Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
17 Code Smells

No Coverage information
0.0% Duplication

Zejnilovic

Code reviewed
Api tested

dk1844 added 19 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

dk1844 requested review from benedeki and AdrianOlosutean as code owners April 4, 2022 14:00

AdrianOlosutean reviewed Apr 5, 2022

View reviewed changes

#1693 PR review update - typos etc

51ad013

dk1844 added the work in progress Work on this item is not yet finished (mainly intended for PRs) label Apr 6, 2022

dk1844 added 2 commits April 6, 2022 17:02

#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

dk1844 removed the work in progress Work on this item is not yet finished (mainly intended for PRs) label Apr 7, 2022

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

Experimental API 3 - Versioned Model #1693

Closed

dk1844 requested a review from AdrianOlosutean April 7, 2022 08:40

dk1844 requested review from AdrianOlosutean, benedeki and Zejnilovic April 19, 2022 07:52

AdrianOlosutean previously approved these changes Apr 26, 2022

View reviewed changes

benedeki reviewed Apr 26, 2022

View reviewed changes

#1693 PR review update 1

09c326e

dk1844 dismissed AdrianOlosutean’s stale review via 09c326e April 28, 2022 13:00

dk1844 requested review from benedeki and AdrianOlosutean May 2, 2022 08:43

AdrianOlosutean previously approved these changes May 2, 2022

View reviewed changes

Zejnilovic reviewed May 3, 2022

View reviewed changes

benedeki assigned dk1844 May 3, 2022

#1693 DatasetService.updateProperties[V3] -> split between DatasetSer…

1c241d1

…vice and DatasetServiceV3 with renaming. VersionList removed in favor of VersionedSummary everywhere. - explanatory comments

dk1844 dismissed AdrianOlosutean’s stale review via 1c241d1 May 3, 2022 11:31

Zejnilovic reviewed May 3, 2022

View reviewed changes

Zejnilovic reviewed May 4, 2022

View reviewed changes

data-model/src/main/scala/za/co/absa/enceladus/model/versionedModel/VersionedSummary.scala Outdated Show resolved Hide resolved

dk1844 added 3 commits May 4, 2022 15:34

#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

Zejnilovic mentioned this pull request May 5, 2022

Consolidate REST API Error handling in api-v3 #2062

Open

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

a80d9ac

…g `dev` or not.

Zejnilovic reviewed May 9, 2022

View reviewed changes

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

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

562da60

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

Zejnilovic approved these changes May 9, 2022

View reviewed changes

benedeki approved these changes May 9, 2022

View reviewed changes

dk1844 merged commit a3fdde4 into develop-ver-3.0 May 9, 2022

dk1844 deleted the feature/1693-api-v3-versioned-model branch May 9, 2022 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/1693 api v3 versioned model - Dataset v3 implementation #2046

Feature/1693 api v3 versioned model - Dataset v3 implementation #2046

dk1844 commented Apr 4, 2022 •

edited

Loading

AdrianOlosutean left a comment

AdrianOlosutean Apr 5, 2022

dk1844 Apr 6, 2022 •

edited

Loading

Zejnilovic May 3, 2022

dk1844 May 4, 2022 •

edited

Loading

dk1844 commented Apr 6, 2022

benedeki Apr 21, 2022

dk1844 Apr 28, 2022 •

edited

Loading

dk1844 May 2, 2022

dk1844 May 3, 2022

benedeki Apr 26, 2022

dk1844 Apr 28, 2022

dk1844 May 3, 2022

benedeki Apr 26, 2022

dk1844 Apr 28, 2022

dk1844 May 2, 2022

dk1844 May 3, 2022

benedeki Apr 26, 2022

dk1844 Apr 28, 2022

dk1844 May 3, 2022

AdrianOlosutean left a comment

Zejnilovic May 3, 2022

AdrianOlosutean May 3, 2022

dk1844 May 6, 2022

dk1844 May 9, 2022

Zejnilovic left a comment

Zejnilovic May 3, 2022

dk1844 May 4, 2022 •

edited

Loading

Zejnilovic May 3, 2022

Zejnilovic May 9, 2022

dk1844 May 9, 2022

sonarcloud bot commented May 9, 2022

Zejnilovic left a comment

-    for {
-      originalValidation <- super.validate(item)
-      propertiesValidation <- validateProperties(item.propertiesAsMap)
-      schemaValidation <- validateSchemaExists(item.schemaName, item.schemaVersion)
-      rulesValidation <- validateRules(item)
+    val originalValidationFuture = super.validate(item)
+    val propertiesValidationFuture = validateProperties(item.propertiesAsMap)
+    val schemaValidationFuture = validateSchemaExists(item.schemaName, item.schemaVersion)
+    val rulesValidationFuture = validateRules(item)
+    for {
+      originalValidation <- originalValidationFuture
+      propertiesValidation <- propertiesValidationFuture
+      schemaValidation <- schemaValidationFuture
+      rulesValidation <- rulesValidationFuture

Feature/1693 api v3 versioned model - Dataset v3 implementation #2046

Feature/1693 api v3 versioned model - Dataset v3 implementation #2046

Conversation

dk1844 commented Apr 4, 2022 • edited Loading

Release notes suggestion

AdrianOlosutean left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dk1844 Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dk1844 May 4, 2022 • edited Loading

Choose a reason for hiding this comment

dk1844 commented Apr 6, 2022

Choose a reason for hiding this comment

dk1844 Apr 28, 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

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

AdrianOlosutean 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

Showcase DEV -Dspring.profiles.active=dev

Showcase Non-DEV

Choose a reason for hiding this comment

Zejnilovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dk1844 May 4, 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

sonarcloud bot commented May 9, 2022

Zejnilovic left a comment

Choose a reason for hiding this comment

dk1844 commented Apr 4, 2022 •

edited

Loading

dk1844 Apr 6, 2022 •

edited

Loading

dk1844 May 4, 2022 •

edited

Loading

dk1844 Apr 28, 2022 •

edited

Loading

Showcase DEV `-Dspring.profiles.active=dev`

dk1844 May 4, 2022 •

edited

Loading