Skip to content

Conversation

@marlewe
Copy link
Owner

@marlewe marlewe commented Nov 8, 2024

This was the origin description in the original pull request:

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

marlewe and others added 27 commits May 7, 2024 12:37
@marlewe marlewe self-assigned this Nov 8, 2024
@marlewe
Copy link
Owner Author

marlewe commented Nov 8, 2024

In the original Pull request this comment was added right after Commit fdfd88e - "some renamings"

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

@marlewe
Copy link
Owner Author

marlewe commented Nov 8, 2024

Status of this redesign iteration:
This code has changed the lists of Exceptions thrown on each API methods. That's not good for documentation generation.
The branch v3_cleaner-ExceptionsForDocumentation uses the abstract IdsException on each API method and called abstract methods to avoid that some methods declaring exceptions that will never be thrown.

But the miredot documentation generator does not recognize the concrete thrown exceptions in the code path of an API method (or does not recognize it till an inheritance depth of 3 - with a depth of 2 it works).

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.

3 participants