adapter: DataImports API and refactoring #322

lunedis · 2021-02-15T11:18:59Z

closes #135

I have a very bad habit of not commiting while I work, so sorry in advance for this mess of a PR consisting of 1 huge commit.

This PR contains the following:

Introduction of the concept of a "Data Import", which is the result of calling /trigger on a datasource. DataImports are saved in the database and can be accessed using new API endpoints (see README and related issue).
Big exception related refactoring: The internal classes (Adapter, DatasourceManager, ...) throw only application-specific exceptions now. Those exceptions are translated into RestResponseExceptions with suitable status code in the endpoint classes. I got a bit carried away and thus this is done using a Map of exceptions to status codes and a fancy method that takes a custom lambda. Check the code review for a caveat.
Some cleanup of imports related to using lombok for e.g. Equals and Hashcode methods.
Updated readme, unit tests and integration tests for all of the above.

CLAassistant · 2021-02-15T11:19:05Z

All committers have signed the CLA.

lunedis · 2021-02-15T11:23:39Z

...r/src/main/java/org/jvalue/ods/adapterservice/datasource/api/rest/v1/DatasourceEndpoint.java

-      throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Datasource needs to exist before updating", e);
-    }
+  public void updateDatasource(@PathVariable Long id, @Valid @RequestBody Datasource updateConfig) {
+    this.handleErrors(() -> { datasourceManager.updateDatasource(id, updateConfig); return null;});


the return null; is necessary as the handleErrors function expects a CheckedSupplier which returns something.

It would be an option to overload that function expecting a (to be defined) CheckedRunnable that returns void, but that would mean more code duplication. It could have been so pretty 😢 ...

sonallux

Nice work introducing application specific exceptions. I really like that, it is much cleaner now!

Instead of using our own handleErrors(...) function, you could also have used these fancy Spring annotations: @ExceptionHandler and @ControllerAdvice. Here and here you can find some tutorials how to use these annotations if you want to go with the spring way of handling exceptions. This is just for reference, for me, it is also ok sticking with the handleErrors(...) function.

georg-schwarz · 2021-02-15T17:47:07Z

adapter/src/main/java/org/jvalue/ods/adapterservice/adapter/api/rest/v1/Mappings.java

 public class Mappings {
  public static final String IMPORT_PATH = "/preview";
  public static final String RAW_IMPORT_PATH = "/preview/raw";
  public static final String FORMAT_PATH = "/formats";
  public static final String PROTOCOL_PATH = "/protocols";
  public static final String VERSION_PATH = "/version";
+
+  public static final Map<Class, HttpStatus> ERROR_MAPPING = Map.of(


I wonder if a status code annotation directly at the exception is better?

@ResponseStatus(code = HttpStatus.BAD_REQUEST) class CustomException extends RuntimeException {}

Probably you had some thoughts on that, do you mind sharing them? :)

I wonder if a status code annotation directly at the exception is better?

Probably you had some thoughts on that, do you mind sharing them? :)

I honestly had no clue about the way you are "supposed" to do it in Spring. Gonna have to take a look at it in detail. The solution I went for was the first one that came to mind to avoid repetition in every endpoint method.

georg-schwarz · 2021-02-15T17:50:50Z

...r/src/main/java/org/jvalue/ods/adapterservice/adapter/model/exceptions/AdapterException.java

+
+import lombok.NoArgsConstructor;
+
+@NoArgsConstructor


I would have assumed that lombok has some help for exceptions, but seems like the PR is not yet merged: projectlombok/lombok#2702

georg-schwarz · 2021-02-15T17:52:29Z

adapter/src/main/java/org/jvalue/ods/adapterservice/datasource/DatasourceManager.java

-    Datasource existing = datasourceRepository.findById(id)
-      .orElseThrow(() -> new IllegalArgumentException("Datasource with id " + id + " not found"));
+  public void updateDatasource(Long id, Datasource update) throws DatasourceNotFoundException {
+    Datasource existing = datasourceRepository.findById(id).orElseThrow(() -> new DatasourceNotFoundException(id));


I think these chains are hard to read.. I guess we should introduce some linter or so in the future xD

I actually kinda like chains like that, guess that comes from my preference for functional programming 😁.

georg-schwarz · 2021-02-15T17:54:01Z

...r/src/main/java/org/jvalue/ods/adapterservice/datasource/api/rest/v1/DatasourceEndpoint.java

@@ -20,52 +22,48 @@
 public class DatasourceEndpoint {
  private final DatasourceManager datasourceManager;

+  private <T> T handleErrors(CheckedSupplier<T> function) throws ResponseStatusException {


duplicate with other endpoints => should we use a convert method and place it next to the mapping?

There are two different Mappings classes though, one for the Adapter, and one for the Datasources.

I have not found a good way to avoid this duplication yet. Someone got any ideas?

georg-schwarz · 2021-02-15T17:55:59Z

adapter/src/main/java/org/jvalue/ods/adapterservice/datasource/model/DataImport.java

+
+  private Date timestamp;
+
+  @ManyToOne(fetch = FetchType.EAGER)


That means the datasource is always also retrieved, right? In general, not a bad idea, but why do we not serialize it then? Just a little confused, do we need it?

We need its id to create the URI to the data in DataImportMetadata.

Also the annotations are necessary for Spring to create the database tables and fields properly I think.

georg-schwarz · 2021-02-15T17:57:33Z

adapter/src/main/java/org/jvalue/ods/adapterservice/datasource/model/Datasource.java

+  @OneToMany(cascade = CascadeType.ALL, mappedBy = "datasource")
+  @JsonIgnore
+  @EqualsAndHashCode.Exclude // needed to avoid an endless loop because of a circular reference
+  private Set<DataImport> dataImports;


When are they fetched? Always on a get? Might lead to a huge overload once we had run 100 imports for a source... Maybe we should not link these concepts at all and just provide a datasourceId in the DataImport?

I put in a fetch = FetchType.LAZY which should resolve your worries about performance.

Maybe we should not link these concepts at all and just provide a datasourceId in the DataImport?

Usually I prefer things doing manually as well, I am not sure if all the Spring SQL magic still works then though. And to be honest, the foreign keys generated by using those annotations are kinda neat.

If we do not want this field, we can safely remove it, because the datasource field in the DataImport class is still annotated with @ManyToOne. This should be sufficient for Hibernate the generate the foreign keys.

I use that field to generate the result of the /datasources/{id}/imports call though. But of course that could also be done using a function in the repository.

Yeah, I would definitely go with a separate findAllByDatasourceId function in the DataImportRespository. This will be much faster because querying all other properties of the Datasource is useless when just return the list of DataImports.

Problem I see with this approach:

We want to differentiate between "return empty array because no data imports were found for the data source" and "return 404 because datasource does not exist".

If we use a function called findAllByDatasourceId it would just return nothing if the datasource does not exist at all.
So we kinda need to fetch the datasource details anyways (or check if it exists, which should not be a big difference).

Ah, I haven't considered that 🙈

Good point.. I am kind of unsure what is best...

I guess lazy loading is misleading when reading code - or at least I'd assume the relation is fetched since I don't work with Spring too often. So personally, I'd not have the reference and make a separate call to prevent these uncertainties from happening. The readability would be clear:

if datasource does not exist => return 404

if imports do not exist => return []

The obvious disadvantage: two database requests instead of one..

The obvious disadvantage: two database requests instead of one..

Well, I think having @OneToMany with FetchType.LAZY will also be two database requests...

lunedis · 2021-02-24T10:51:52Z

I updated the PR and changed the exception handling to be more spring-y (?, springful? springesque?).

@sonallux could you please take a look?

Also that was a very short endeavor into the functional side of Java... 😢

sonallux

Just two very minor things, otherwise LGTM. The exception handling is now much more springish 👍

This PR also fixes #232 🚀

...r/src/main/java/org/jvalue/ods/adapterservice/datasource/api/rest/v1/DataImportEndpoint.java

georg-schwarz · 2021-02-25T10:17:38Z

Nice! I think this feature is a good step forward :)

adapter: data imports

b892a30

lunedis self-assigned this Feb 15, 2021

lunedis requested a review from georg-schwarz February 15, 2021 11:19

lunedis commented Feb 15, 2021

View reviewed changes

adapter: fix compile issue related to typo on Getters

04dce65

sonallux reviewed Feb 15, 2021

View reviewed changes

georg-schwarz reviewed Feb 15, 2021

View reviewed changes

lunedis added 2 commits February 16, 2021 11:08

adapter: use lazy loading for data imports

39c366a

adapter: refactor exception handly to be more springy

8abef34

georg-schwarz previously approved these changes Feb 24, 2021

View reviewed changes

sonallux reviewed Feb 24, 2021

View reviewed changes

...r/src/main/java/org/jvalue/ods/adapterservice/datasource/api/rest/v1/DataImportEndpoint.java Outdated Show resolved Hide resolved

...r/src/main/java/org/jvalue/ods/adapterservice/datasource/api/rest/v1/DataImportEndpoint.java Outdated Show resolved Hide resolved

adapter: simplify mappings for DataImportEndpoint

a97276c

lunedis dismissed georg-schwarz’s stale review via a97276c February 25, 2021 07:36

lunedis requested a review from georg-schwarz February 25, 2021 07:50

sonallux approved these changes Feb 25, 2021

View reviewed changes

georg-schwarz approved these changes Feb 25, 2021

View reviewed changes

georg-schwarz merged commit c2aa19d into main Feb 25, 2021

georg-schwarz deleted the adapter-data-import-main branch February 25, 2021 10:17

sonallux mentioned this pull request Feb 25, 2021

Adapter: ResponseStatusException message is not present in response body #232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adapter: DataImports API and refactoring #322

adapter: DataImports API and refactoring #322

lunedis commented Feb 15, 2021

CLAassistant commented Feb 15, 2021 •

edited

Loading

lunedis Feb 15, 2021

sonallux left a comment •

edited

Loading

georg-schwarz Feb 15, 2021

lunedis Feb 16, 2021

georg-schwarz Feb 15, 2021

georg-schwarz Feb 15, 2021

lunedis Feb 16, 2021

georg-schwarz Feb 15, 2021

lunedis Feb 16, 2021

georg-schwarz Feb 15, 2021

lunedis Feb 16, 2021

georg-schwarz Feb 15, 2021

lunedis Feb 16, 2021

sonallux Feb 16, 2021 •

edited

Loading

lunedis Feb 17, 2021

sonallux Feb 17, 2021

lunedis Feb 17, 2021

sonallux Feb 17, 2021

georg-schwarz Feb 17, 2021

sonallux Feb 17, 2021

lunedis commented Feb 24, 2021

sonallux left a comment

georg-schwarz commented Feb 25, 2021


		import lombok.NoArgsConstructor;

		@NoArgsConstructor

adapter: DataImports API and refactoring #322

adapter: DataImports API and refactoring #322

Conversation

lunedis commented Feb 15, 2021

CLAassistant commented Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

sonallux left a comment • 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

sonallux Feb 16, 2021 • 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

lunedis commented Feb 24, 2021

sonallux left a comment

Choose a reason for hiding this comment

georg-schwarz commented Feb 25, 2021

CLAassistant commented Feb 15, 2021 •

edited

Loading

sonallux left a comment •

edited

Loading

sonallux Feb 16, 2021 •

edited

Loading