Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V3 file info improvement and request handler #157

Open
wants to merge 96 commits into
base: v3
Choose a base branch
from

Conversation

LewerenzM
Copy link
Contributor

@LewerenzM LewerenzM commented Mar 13, 2024

This branch is a suggestion for the first iteration of a redesign. It is based on a new common base class for data sets and data files and a request handler approach to separate the different concerns in IdsBean to concern related handler classes.
To reduce the distinction between the 3 possible StorageUnit configurations inheritance is used in DataSelection and FiniteStateMachine.

Advantages:

  • For each request type we now have a separate request handler class
  • Helper code now only appears in the classes where it is needed.
  • Less if/else blocks for distinction of StorageUnit
  • DataInfoBase class made method signatures more common and reduced a lot of code
  • StorageUnit depended base classes (DataSelection and FSM) encapsulating differences
  • Smaller more readable code units
  • Less redundant code

Disadvantages:

  • Parameter mapping (and back) into ValueContainer map for each request.
    • Why not handing over the whole request in handle() method? Because for put request an additional parameter is used and in putAsPost() a multipart request would have to be converted to a single part request.
  • Additionally list conversion in DfRestorer.run() .
  • Several casts into DataSetInfo and DataFileInfo.
    • Possibly move that code into the DataInfoBase subclasses themselves, but this may will lead to unclear and confusing data model classes and would be a mix between logic and data model (no SoC).
  • The ServiceProvider is a solution for the redesign process. But it maybe should be deleted to move back to dependency injection (better testing/mocking).

Architecture

firstIterationArch

@LewerenzM
Copy link
Contributor Author

LewerenzM commented May 16, 2024

I tried to consider most of the feedback. Following changes were made:

Request handler changes

The whole request handler approach got a little redesign. The RequestHandlerService was removed and now, the IdsService creates and calls the specific handlers by itself.
The different parameters for each requests are handed over by calling the constructors.

To have the IdsService as simple as it is possible and to avoid repetitive code, several tasks like deciding if we have to deal with prepared or with unprepared data, or the transmitting, is outsourced to the request handler infrastructure.

The RequestHandlerBase class is the highest base class and triggers a pre and a post processing in its handle method. After the preprocessing the handleRequest() method is called. All handlers which do not need a DataSelection at first are deriving from the RequestHandlerBase and have to implement the handleRequest() method.
All child classes are able to overwrite the method addParametersToLogString() for optional adding some information for the log output of this request.
Additionally the transmitting (post processing) is triggered in the base class. Child classes can overwrite addParametersToTransmitterJSON() to add additionally information to the transmitted JSON string.

All request handlers which have to create a DataSelection at first, are now deriving from DataRequestHandler. Itself derives directly from RequestHandlerBase and provides some additional functionality. The abstract method handleDataRequest(DataSelectionService) has to be implemented by all child classes and the DataRequestHandler will create the DataSelection implicitly.
The DataRequestHandler has a DataControllerBase property. It depends if the sessionId is null if a concrete UnpreparedDataController or a PreparedDataController is created. The data controllers providing the DataSelectionService in the right way and have methods to enrich the log entries and the transmitter JSON with additional information, like preparedId, sessionID or InvestigationIds.

The methods addParametersToLogString() and addParametersToTransmitterJSON() are implemented by the DataRequestHandler and the additional information from the DataControllerBase is considered here.
To make it possible that concrete DataRequestHandler implementations are also able to add information for the log entry or the transmitter JSON the methods addCustomParametersToLogString() and addCustomParametersToTransmitterJSON() can be overwritten, like it is done in GetDataHandler.

secondRequestHandlerApproach

Changes for DataSelection

In a first step the DataSelection creation was seperated from its logic which is needed while request processing. So the classes DataSelectionFactory and DataSelection were created.
In this iteration the DataSelection was splitted again to seperate conserns: Now we have the DataSelectionService for the logic and the DataSelection for just containing the data and providing some simple functionality like mustZip(). The DataSelectionService consists of its methods and one DataSelection object. The name of the factory now is DataSelectionServiceFactory and it creates the DataSelectionService. Since the factory provides the service, the developer only works with the DataSelectionService directly. All DataSelection methods are encapsulated in the service.

splittingDataSelection

@RKrahl RKrahl mentioned this pull request Jul 31, 2024
@LewerenzM LewerenzM force-pushed the v3_FileInfoImprovementAndRequestHandler branch from e1f28ea to 33135c3 Compare August 7, 2024 08:29
@LewerenzM
Copy link
Contributor Author

LewerenzM commented Aug 7, 2024

Had reset the branch to my latest commit, because of a merge from v3 which was planned later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants