Re-grouping of package structure for controller and output classes

[FabiKo117]

When starting with the implementation of #115 I ran into the following issue: We have a ContributionsController under the contributions package and then other controller classes like CountController, AreaController, etc. under the dataaggregation package. Now I want to add a data aggregation endpoint to the contributions.

Since we are still working with contributions and need the contribution-view of the OSHDB here too, I'd suggest to add another package level.

Current package structure:

• controller
  • contributions
    • ContributionsController.java
  • dataaggregation
    • AreaController.java
    • CountController.java
    • ...
    • UsersController.java
  • metadata
    • MetadataController.java
  • rawdata
    • ElementsController.java
    • ...
  • other helper classes

Proposed new package structure:

• controller
  • contributions
    • ContributionsController.java
  • elements
    • dataaggregation
      • AreaController.java
      • CountController.java
      • ...
    • features
      • ElementsController.java
      • ...
  • users
    • UsersController.java
  • metadata
    • MetadataController.java
  • other helper classes

The new package structure would be in-line with our endpoint structure and in my opinion a better grouping. As the aggregation functions that can be applied on the contributions are rather limited, I'd suggest adding them to the ContributionsController (like we've done it with the UsersController as well) and not make another CountController inside the contributions package. Feedback appreciated.

[bonaparten]

I think reorganizing the package structure for the controller is a good idea. The actual package structure is misleading.

[FabiKo117]

Yes, I think so too. When taking a look at the executor and output package structure, I think there are also some restructuring actions necessary. I will also work something out for those packages and post it then here.

[FabiKo117]

For the executor package, we would then mostly end up with the situation that we have one class per package and then the helper classes used in different classes on the outside-level. So we would not gain much from a restructuring there.

However, for the output, it currently looks like that:

• dataaggregationresponse
  • elements
    • ElementsResult.java
  • groupbyresponse
    • GroupByObject.java
    • GroupByResult.java
    • RatioGroupByBoundaryResponse.java
    • ...
  • users
    • UsersResult.java
  • Attribution.java
  • DefaultAggregationResponse.java
  • Response.java
  • Result.java
  • ....
• metadataresponse
  • ExtractRegion.java
  • ...
• rawdataresponse
  • DataResponse.java
• Description.java

Here is actually also the issue, that some classes are grouped falsely, e.g. the two interfaces Response.java and Result.java are used over all responses and not just the data-aggregation responses. So a restructuring is definitely necessary within the output package.

Proposed new structure:

• contributions
  • ContributionsResult.java (new class coming potentially through PR implementing /contributions/count endpoint #121)
• elements
  • ElementsResult.java
• metadata
  • ExtractRegion.java
  • MetadataResponse.java
  • TemporalExtent.java
• groupby
  • GroupByObject.java
  • GroupByResponse.java
  • GroupByResult.java
• ratio
  • RatioResponse.java
  • RatioResult.java
  • RatioGroupByBoundaryResponse.java
  • RatioGroupByResult.java
• DataResponse.java, Description.java, Response.java, Result.java, Attribution.java, DefaultAggregationResponse.java, Metadata.java are all outside in the output package directly

With this structure, we will be again in-line with the different endpoints, together with having 2 additional grouping packages (groupbyresponse and ratioresponse). The other classes that are now in the output package directly are used across several (or all) other packages.

edit1: removing users package + class, as it has the same structure as ContributionResult.java, which will then be used instead for the /users response creation
edit2: removing the response from the end of the package names, e.g. renaming ratioresponse to ratio
edit3: moving RatioGroupByBoundaryResponse.java and RatioGroupByResult.java to the ratio package

[tyrasd]

controller package

The new package structure would be in-line with our endpoint structure and in my opinion a better grouping.

I really like the idea to make them be in-line with each other. 👍

But why call it rawdata and not something a bit more specific? e.g. related to feature or geometry?

As the aggregation functions that can be applied on the contributions are rather limited, I'd suggest adding them to the ContributionsController (like we've done it with the UsersController as well) and not make another CountController inside the contributions package. Feedback appreciated.

Here, I would personally tend to split it into a dedicated class for aggregations like Count, but if you think that working on a single class makes the implementation cleaner or easier, I would not object that solution either.

output package

the proposed structure sounds good as well. The only(?) difference is that there is no distinction in the elements package towards dataaggregation and "raw data"… Is that because there are no dedicated data structures for data extraction since we use the geojson library directly for that? 🤔

PS: The groupbyresponseand ratioresponse could also live in the elements package, right? Because I could imagine that at some point there could also be groupBy resources in the contributions endpoints, for example.

[FabiKo117]

But why call it rawdata and not something a bit more specific? e.g. related to feature or geometry?

Good point, something like features would be more meaningful. I'll update that in the suggested format above.

Here, I would personally tend to split it into a dedicated class for aggregations like Count, but if you think that working on a single class makes the implementation cleaner or easier, I would not object that solution either.

Yeah I was a bit unsure about that one again last week when I was working more with contributions/count. There are some limitations on how we can define the swagger structure via the controller classes. I was playing around a bit with that last week and think I hit a wall when trying to define a different set of parameters for two end points that are in the same controller class, which would be the case and need for /contributions and /contributions/count (without specifying everything manually via annotations before every endpoint of course). So I guess it could end up that we have to go for a multi-class solution here if we want to stick with swagger how we use it at the moment.

Is that because there are no dedicated data structures for data extraction since we use the geojson library directly for that?

We have here the class DataResponse.java, which is used for those responses. But as it is used by different endpoints like /elements and /contributions, I would put it on the outer package output.

PS: The groupbyresponse and ratioresponse could also live in the elements package, right? Because I could imagine that at some point there could also be groupBy resources in the contributions endpoints, for example.

The groupByResponse is already used across different endpoints, e.g. also /users uses it. The same goes for the ratioResponse. That's why they have their own package and are not within the other ones.
edit: about the groupBy option for contributions: I think it could also work with the already available classes, like the GroupByResponse, but in case we would need a new one, indeed it's probably best to put that in the contributions package. I will then also move the respective Ratio classes into the ratio package, as that would make more sense to only keep the generic groupBy classes in the groupby package.

[bonaparten]

What do you think about structuring output like this:

Description.java
Result.java
Response.java
Attribution.java
contributions

• ContributionsResult.java (new class coming potentially through PR #121)
  

dataaggregation

• ElementsResult.java
  

• UsersResult.java
  

• DefaultAggregationREsponse.java
  

dataextraction

• DataResponse.java (maybe renaming this class?)
  

metadata

•  Metadata.java
  

• ExtractRegion.java
  

• MetadataResponse.java
  

• TemporalExtent.java
  

groupbyresponse

• GroupByObject.java
  

• GroupByResponse.java
  

• GroupByResult.java
  

• RatioGroupByBoundaryResponse.java
  

• RatioGroupByResult.java
  

ratioresponse

• RatioResponse.java
  

• RatioResult.java
  

Description.java, Result.java, Response.java, and Attribution.java are not in a subpackage of output.

[FabiKo117]

@bonaparten thanks for your suggestion.

about metadata: the class Metadata.java does not only have something to do with the /metadata response, to which this package is refering to, but with all responses. So it does not fit in there. It could also be renamed to something like ResponseMetadata.

about DataResponse: we could also put that class in a distinct package and potentially rename it yes, as @tyrasd also suggested with the controller. Something like FeatureResponse then maybe?

The main difference that I see then is that you use a dataaggregation package and add DefaultAggregationREsponse.java as well as a dataextraction package. I would not like that structure, because the ContributionsResult.java is also used data-aggregation responses (e.g. /contributions/count), but is located in another package. That's why I want to go for a package naming and grouping that fits to the names of our endpoints and also groups some other classes together that have things in common (groupBy and ratio).

[bonaparten]

about metadata: the class Metadata.java does not only have something to do with the /metadata response, to which this package is refering to, but with all responses. So it does not fit in there. It could also be renamed to something like ResponseMetadata.

Metadata.java could be put outside the metadata package and not having a subpackage.

about DataResponse: we could also put that class in a distinct package and potentially rename it yes, as @tyrasd also suggested with the controller. Something like FeatureResponse then maybe?

maybe DataExtractionResponse.java?

Anyway, I am happy also with your solutions. if you prefer one structure more than another it is ok for me :D. There is not a perfect way to structure packages. We know we should change the package structure, this is the main thing.

[FabiKo117]

Metadata.java could be put outside the metadata package and not having a subpackage.

👍

maybe DataExtractionResponse.java?

This class is used for the /elements/geometry response, as well as the /contributions response, so maybe FeatureResponse and DataExtractionResponse are both not so fitting.. what about ExtractionResponse?

[bonaparten]

ExtractionResponse could also be ok.
Independently of the naming, we should add some comment lines to the Javadoc of the class. The doc is misleading if the class is used for other endpoints:

Represents the whole GeoJSON response object for the data-extraction endpoints.

[FabiKo117]

How about the following:

Represents the whole GeoJSON response object for the data-extraction and contribution endpoints that always extract OSM data as GeoJSON, e.g. /elements/geometry or /contributions/geometry.

I would add the always as you can also get GeoJSON through other endpoints like /groupBy/boundary, but there it's optional.

[bonaparten]

Represents the whole GeoJSON response object for the data-extraction and contribution endpoints that always extract OSM data as GeoJSON, e.g. /elements/geometry or /contributions/geometry.

it is fine

[FabiKo117]

Due to the limitations given by Swagger in regards to the controller packages, I had to make some adaptions in that structure. Please have a look at the structure as it it now in #124.

[bonaparten]

It would be also nice to rename the classes AggregateRequestExecutor.java and DataRequestExecutor.java in AggregationRequestExecutor.java and ExtractionRequestExecutor.java.
So these classes will follow the same naming criteria used in other packages, i.e. in output: ExtractionResponse.java and DefaultAggregationResponse.java

[FabiKo117]

It would be also nice to rename the classes AggregateRequestExecutor.java and DataRequestExecutor.java in AggregationRequestExecutor.java and ExtractionRequestExecutor.java.
So these classes will follow the same naming criteria used in other packages, i.e. in output: ExtractionResponse.java and DefaultAggregationResponse.java

Agree, there are also other parts of the code, where some renaming/restructuring would be useful. We could potentially discuss about all these parts in another issue and then work on them step-by-step.

Can you create an issue for that @bonaparten?