Refactoring Week 12/2021

[MoritzKeppler]

We'd like to have one week beginning of december focussing on housekeeping, core architecture discussions and refactoring.
Add whatever you think we should take care of to this discussion and let's find out what needs to get improved first.
_{Moritz Keppler [email protected], Daimler TSS GmbH, legal info/Impressum}

[MoritzKeppler]

I'd like to rethink the way we order and load extensions, see eclipse-edc/Connector#121
Maybe we just need to improve the docs, maybe we can simplify / have to align some stuff.

[DominikPinsel]

If we are doing this maybe we can also address the cyclic extension dependency in core-contract and contract-offer-framework. Also we should document somewhere, that cyclic dependencies between extensions are forbidden.

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[alexandrudanciu]

Regarding the topic of loading extensions I would like to address the edge case of loading multiple extensions contributing the same type of service (e.g., WebService) and how the EDC should behave (replace the previous one in the map, throw exception, log info, etc.).

[DominikPinsel]

I'd like the rework the TransferProcess class. I think it leaks too much information. In general the SPI extension shouldn't need to know something about the state machine, the core-transfer extension uses. I see this as an implementation detail that should not be leaked to the SPI.

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[jimmarino]

The state machine is fundamental to the system design and is what the protocol between the consumer and provider is based on. It needs to be in the SPI and reflected in the TransferProcess because everything is built on processes asynchronously transitioning between states.

[DominikPinsel]

In my eyes the state machine is just ne implementation detail of the TransferProcessManager. It should not matter for other components whether this class uses a state machine, some event mechanism or any other method to manage the transfer process.
I think the knowledge about a state machine should not be leaked to other components, as it increased the size of the transfer interface(s) and therefore adds to the overall complexity of the whole transfer process.

[DominikPinsel]

As it is now I would remove the contract negotiation states from the SPI, too.

[DominikPinsel]

I'd like to remove sample code. We are adding a lot of sample code every week, I assume large parts of this sample code is not needed anymore and should be removed.

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[DominikPinsel]

@MoritzKeppler showed in issue #92 how the folder structure can be improved. I still think this is a good idea and we should do it. Maybe we can use the refactoring week for this.

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[DominikPinsel]

Combine all memory store extensions into one.

We have several extensions that implement some kind of memory store functionality. Maybe we can combine them all into one extension and make the service registration configurable. Like this we don't get some kind of new memory store extension, every time a new kind of data needs to be stored. Also they are probably used in combination anyway.

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[paullatzelsperger]

How would you then only use one in-mem impl as opposed to: all of them?

[DominikPinsel]

Sorry I'm not sure whether I understand the question. But let me try to answer it anyway.

I assume the in-memory extensions are something that may be used to run a sample/integration test without too much hassle. In most of these cases you need multiple or all in-memory implementations at the same time. For these two cases it would be easier to include only one extension instead of many.
To enable the user run a mix of in-memory and non-in-memory storage solutions I would suggest to make it configurable. E.g. to disable the in-memory AssetIndex and include another extension, that implements this store, too.

Pros:

• 1 in-memory extension instead of 9 (atm)
• Less code necessary to add new in memory store
• Less extensions to include in case someone wants run a sample/test
• Less samples/tests to update in case we get a new in-memory extensions

Cons:

• In case someone wants to mix in-memory and non-in-memory extensions this needs to be configured in the launcher

[alexandrudanciu]

I would like to discuss if a service extension is meaningful if it doesn't implement any service interface specified in any SPI module. An example would be catalog-service.

[jimmarino]

Yes, I think it is. For example an API extension. Or, a set of modules that have their own SPI and only consume services in the runtime, e.g. observability extensions.

[DominikPinsel]

Maybe we can also talk about how to implement the builder pattern. I like it when the object is created in the build() method, but currently a lot of implementations of the pattern create the object in the constructor and then fill its properties. The build() method then just returns the object.

[jimmarino]

The builder pattern we implemented is designed to work around Java's lack of named parameters, not to enforce immutability. With that in mind, having separate parameters on the builder that are just copied to the object in the build() method seems like a lot of additional ceremony and typing.

[juliapampus]

One further topic: the agreement on naming. We again have both terms in the code, consumer and client.

[jimmarino]

+1 I think we already agreed on naming. I may be the culprit in some of the confusion. We should just clean up the inconsistencies.

[ndr-brt]

Proposal: abstract Result concept

Description

There's a lot of classes in the codebase that represent the concept of a Result, something returned by a component that could be a valid content or an error (with details)

Scope

All the codebase

Benefit

Fo3r EDC developers it will avoid to have to reinvent the wheel everytime they need to return something that could be a valid content or an error

Risk

Can't see any

[paullatzelsperger]

like the idea

[jimmarino]

This sounds like a good idea but it may be one of those things we have to try out and see if it makes sense by working through a number of examples.

[ndr-brt]

#363

[mspiekermann]

To prepare the tpopics for our discussion please provide a summary with the following structure:

1. Description of the proposal and motivation
2. Technical impact and scope of the proposed change in the codebase. For example, which subsystems does it affect, what is the extent of the changes required?
3. User benefit derived from the proposed change. The user benefit could be to EDC developers or "end-user" developers/operators that consume the EDC (or potentially other interested parties).
4. Risk assessment and potential destabilization to the code base

[DominikPinsel]

This is not really a code refactoring, but maybe it's still something we could do next week.
I think we could also do a little refactoring/retrospective of our working model. There are probably some things we can improve, so that we can work faster/more/with better quality efficient in the future. Especially the last two weeks were a little bit chaotic and showed me some areas we can improve.

[denisneuling]

Proposal: Introduction of lombok

Description

Lombok is a java library to reduce the amount of boilerplate needed to be written.

Impact / Scope

Will basically affect all POJOs and value containers across all modules if used consequently.

Benefit

Avoids boilerplate that take the focus off the important stuff.

Risk

Improper usage might lead to side effects (e.g. improper hashcode/equals code generation)

_{Denis Neuling [email protected], Daimler TSS GmbH, legal info/Impressum}

[paullatzelsperger]

While I like lombok in an app-development scenario, I'm less sure about it in a platform scenario, which is what we're facing. It could bring nasty version clashes for very little benefit IMO.

Another consideration I have is that it's a broadsword rather than a scalpel, and it doesn't really work well unless classes and fields are annotated in a very specific and detailed way, and personally I prefer methods over a list of annotations.

I'd be willing to give it a try if we can guarantee that the code does not change, meaning, the annotations must be made such that the same amount (and semantics) of getters, setters, builders, etc. is retained.

[jimmarino]

Lombok introduces modifications to the Java language itself and includes many features we do not want, such as its own approach to logging, synchronization, getters, exceptions, etc. The codebase would not compile without the Lombok plugin. This also makes debugging difficult given a significant portion of the runtime code would be generated.

Even if we were to use it solely for builders (which we could not restrict), such a change would bring an enormous disruption to the codebase as it would alter any class that uses a builder.

What real benefit does this bring? Does it really reduce that much boilerplate code? Given many of the builders need to do validation, I don't think it would reduce that much in the end.

[denisneuling]

Proposal: Replace checkstyle by spotless

Note: Issue 378 has been created.

Description

While checkstyle is doing good in verifying contributions stick to our code style guide, it is not able to actually apply those.
Fixing code style violations manually is a time consuming and cumbersome task, which can be avoided using spotless (./gradlew spotlessCheck || ./gradlew spotlessApply).

Impact / Scope

• Existing CI integration will be impacted and needs adaption
• Our styleguide.md might need to be adapted
• Whole codebase might be impacted if there are feature gaps between checkstyle and spotless

Benefit

Gives developers a tool to automatically verify and reformat code, which will boosts productivity due to manual code style alignment avoidance.

Risk

• Will need some effort to be introduced depending on how much our code style guide was deriving from Google Java Style, potentially we have to introduce custom formatting steps
• Whole code base reformatting might be needed

_{Denis Neuling [email protected], Daimler TSS GmbH, legal info/Impressum}

[paullatzelsperger]

The major advantage I see over the Checkstyle+SaveActionsPlugin combination is that with spotless there is one style config for both. However, devs would still be responsible to execute the spotlessApply task before committing, otherwise the CI build would fail.

the other bothersome thing is IntelliJ integration. I could not find a proper way to integrate this via a plugin. Not a showstopper, just something to consider.

And again, we must ensure that the emerging code style is congruent to what we have at the moment.

[jimmarino]

If this can be done while preserving all of the checks and style we currently have, and if someone wants to spend time on it, why not?

[ndr-brt]

Proposal: substitute EasyMock with Mockito (see #220)

Description

EasyMock is pretty annoying to use for these three reasons:

• every mock needs the call of the replay method
• void methods require that expectLastCall() that's not that easy to read at first (it tooks me some time to get it)
• the behaviour setting and the behavior verification are made in the same place (at expect), and that makes the tests less readable.

Replacing it with Mockito would resolve all of these 3 issues

Scope

Pretty much all the test classes of all the extensions

Benefit

Tests will be more concise and elegant

Risk

Since the people here are used to EasyMock, even if the api are pretty much the same the transition could be still painful.

[jimmarino]

Mockito may be better but my concern is this will introduce a significant risk: all of the tests will need to be re-written and verified. That will take a significant amount of time. Also, who is able to verify all tests across the entire codebase? This will involve many different people and we will need multiple verifications per test.

Given the amount of work this effort would require, what benefits does it bring other than slightly less verbose tests (by a couple of lines in most cases) and an arguably more elegant way of testing?

[ndr-brt]

I don't think the tests need to be rewritten, just a matter of changing api:

• replace EasyMock.expect call with Mockito.when
• replace EasyMock.verify with Mockito.verify adding the expected call on the verification (copy/paste from expect essentially)
• replace the EasyMock.expectLastCall with the Mockito.doNothing().when(...) call
• remove all the EasyMock.reply calls

Seems like not a big task to deal on, it can be done also with multiple PRs, and I'm pretty sure that this will lead people to write better tests

[ndr-brt]

#220

[alexandrudanciu]

Description
Loading multiple extensions contributing the same type of service (e.g., WebService) would result in replacing the previous one in the corresponding map (cf. DefaultServiceExtensionContext#registerService on line 125).
The proposal is to either throw an exception during the startup, or at least to write warnings to the console.
 
Impact / scope
The change would mainly affect the bootstrap module.
 
Benefit
The operator would benefit by receiving the awareness of potential inconsistencies and differences in the behavior of the connector.
 
Risk
Minor

[jimmarino]

Issuing a warning sounds like a good idea

[ndr-brt]

So #124 can be closed?

[ndr-brt]

Proposal: give extension a name

Description

At the moment every ServiceExtension has to log the Initializated/Started/Shutdown events, but that could be done by the ServiceExtensionContext and by the ExtensionsLoader.
The ServiceExtension could have a name() method that returns by default the class name but it could also be overwritten.

Scope

the ServiceExtension class and then all the implementations

Benefit

Less code, the service init/start/shutdown logging phase will be handled in a single place, no need to remember to add logs on the service extensions.

Risk

None

[jimmarino]

I really like the idea of introducing name(). However, I think it should be a method that must be implemented as opposed to introducing an abstract class into the ServiceExtension hierarchy.

[ndr-brt]

There's no need to introduce an abstract class, the default method can lie in the ServiceExtension interface

[jimmarino]

How is it going to return the class name then? Default methods do not have access to the "this" construct.

[ndr-brt]

yes, they do, just tried

[ndr-brt]

eclipse-edc/Connector#358

[ndr-brt]

Proposal: add CommandQueue to TransferProcessManager

There's a discussion goin' on here: #330

[jimmarino]

We need to do this, although I'm not sure it's part of the refactoring work. The implementation itself will likely be simple but there are some scenarios and sequences we need to carefully think through which leads me to think this is something for the next Milestone release.

[jimmarino]

Here are @paullatzelsperger and my proposals. One thing to note is we included a discussion on establishing a framework for refactoring. That section is independent of our specific proposals that follow. We think it is important to have a staged process to the refactoring where smaller changes precede more extensive changes that will have to be introduced sequentially.

Regarding our specific proposals, our focus is on not just technology changes but also documentation and samples. We believe we need to focus much of the effort on things that drive tangible end-user benefit. Also, looking at the proposals above, we think many of them will fit well into this approach.

Establishing a Framework

We believe the goal of refactoring week should be to improve the quality of the software we are delivering to our users. This can be broken down into four key areas:

1. Fix critical Milestone 1 issues
2. Testing and delivery pipeline improvements
3. Documentation
4. Relieve technical debt that significantly impacts project goals

This work should also be organized and structured so that changes can be efficiently and safely introduced into the project. Our proposal would be to front-load and parallelize tasks that can be completed in isolation and without disrupting the work of others. Other tasks (e.g. module restructuring) should be done at the end when other work has been completed.

Our proposals

1. Left Operand Constraint limitations in the InfoModel library.

Category: Critical Milestone 1 issue

Description

The InfoModel library defines left operands using a Java enum. This means that when the EDC uses IDS, the only policy constraints that can be expressed are the 17 that are defined in the enum. This is a critical limitation as it is not IDS compliant and restricts the ability of end-users to define anything but rudimentary usage policies.

Proposed Solution

A fix from the InfoModel team will likely take some time. Given the severity of the issue, our proposal is to temporarily fork the InfoModel library by creating a copy in the EDC repo and using it. The copy will keep the existing InfoModel package names and structure to facilitate migration back to the InfoModel library once the issue has been fixed. The left operand enum will be converted to a type that takes a String.

Impact

This change will have minimal impact on the EDC codebase. Changes will be confined to IDS module dependencies and two transformer classes (ExpressionToIdsLeftOperandTransformer and IdsLeftOperandToExpressionTransformer). This change can be done in parallel to other tasks.

2. Samples Cleanup

Category: Documentation

Description

The samples do not demonstrate new features delivered in Milestone 1.

Proposed Solution

Perform the following tasks:

A. Verify all samples work
B. Update existing samples to demonstrate new features in Milestone 1 including: contract offers, policies, contract negotiation, and data loading

Impact

Geat end-user value helps drive adoption of the EDC. Does not impact core code.

3. Control API Documentation

Category: Documentation

Description

The Control API should have Swagger/OpenAPI documentation. End users have

Proposed Solution and Impact

Add annotations to the Control API endpoints. No code impact.

4. Policy Documentation

Category: Documentation

Description

Document what Policies are:

• Explain the different types (Duties, Permissions, Restrictions)
• Explain the PolicyModel (Constraints, Rules, etc)
• Show how to build policies using Builders
• Explain the PolicyEngine, how the evaluation works

Proposed Solution and Impact

Have someone that has not worked on the policy subsystem learn and document it. No code impact. This will benefit end-users who will need this information soon.

5. DataLoader Documentation

Category: Documentation

Description

Explain how the Dataloader is used and how it can be extended. Discuss how *.json files are imported and how the DataLoader can be used to implement a "data loading server."

Proposed Solution and Impact

Have someone that has not worked on the loader subsystem learn and document it. No code impact. This will benefit end-users who will need this information soon.

6. Remove extension phases and standardize usage of provides/requires

eclipse-edc/Connector#194

Category: Technical Debt

Description

Primordial and Default extensions are a relic from before extension dependency loading was introduced. This mechanism can be replaced by loading extensions. We also need to standardize on what level of granularity requires and provides should use.

Proposed Solution

We propose to:

• Remove primordial and default extensions and replace them with appropriate provides/requires statements
• Introduce the concept of "default" requires modules. Default requires is similar in concept to the java.lang package. Anything essential for the connector runtime
  (e.g. TPM, CNM) would be added as a default requires to modules loaded in the system.
• Provides/Requires would be feature-based. Specifically, a module would not need to require two individual services if they were part of the same feature; they would require
  the feature. Two or more services are part of the same feature if they are always implemented by the same module.

Impact

This will touch all extensions and will therefore have a significant impact on the codebase. This refactoring should be done at the end of the week. This change would mostly benefit developers extending the EDC and has limited value beyond that.

7. Integration Test Stability

Category: Testing and delivery pipeline improvements

Description

We're all aware of the problems with the integration tests.

Proposed Solution

Fix them.

Impact

This will likely impact the rate of other check-ins. Therefore, this work should be done towards the end.

8. Introduce a Foundation Module

Category: Technical Debt

Description

When implementing the contract negotiation subsystem, multiple services needed to be copied from the transfer process module. These services should be consolidated into a third foundation module that the others depend on.

Impact

This will clean up code duplication and needs to be done at some point before new work begins on the CNM.

9. Split up core SPI

Note this is related to 10 but should be done first

Category: Technical Debt, Project Direction

Description

The EDC project is growing to encompass more services than just the connector. At some point, we will likely need to break the codebase into separate repositories for individual services.

Proposed Solution

The SPI should reflect this division:

• A core SPI common to all EDC services. This would include the service loader extensions and any services common to all runtimes (e.g. IdentityService)
• A Connector SPI including the TPM and CNM
• A Catalog SPI
• A Service registry API

Impact

This will require module reshuffling but has relatively low risk. The existing SPI module can be used as a BOM to make it easy to deploy all services in a single runtime. This refactoring should be done towards the end of the cycle.

10. Restructure the Project Module Layout

eclipse-edc/Connector#92

Category: Technical Debt, Project Direction

Description

We should arrive at a specific module layout but it should also include the distinction between:

• Core modules common to all runtimes
• Connector modules
• Catalog modules
• Service registry modules

Proposed Solution

Combine the proposal in #92 with a division along the 4 categories outlined above.

Impact

This will require module reshuffling but has relatively low risk. This should be done at the very end of the cycle, and after 8 & 9.

11. Added Bonus: Remove Old IDS and Sample Modules

Category: Technical Debt

Description

There are old IDS and sample policy modules including a PolicyRegistry.

Proposed Solution

Delete them

Impact

This may impact some Catena-X teams, e.g. the group working on the PRS prototype. We would need to liaise with them.

[ndr-brt]

Agree with everything, I have some points for the discussion:

2

I think that all the "onboarding samples" should run in a local environment, so in my opinion the number 05 should be changed to permit anyone to run it without the need for an aws/azure account (using local containers like azure-cosmos-emulator and minio)

7

Like for the number 2, I think that also integration tests should not lean on aws/azure environments.
Like we did with omejdn in #260 . This would permit anyone to run tests locally.

[ndr-brt]

Number 3 is described by #46

[bscholtes1A]

Proposal: Decouple read and write in Data Flow Controller

Description

In todays implemenetation, a DataFlowController implementation consists of a pair reader/writer:

• reader is in charge of retrieving the data on provider side, e.g. from a S3 bucket.
• writer is in charge of copying the data at destination on consumer side, e.g. into an Azure blob storage

Let's imagine we have two DataFlowController already implemented in the code base:

• one transferring from Azure blob storage to s3 bucket
• one transferring from s3 bucket to Azure blob storage

Then we wont have the possibility to transfer from Azure blob storage to another Azure blob storage without coding a new DataFlowController.

The idea would be to have a catalog of readers/writers and offer the possibility to compose the DataFlowController.

Impact / Scope

Should be null

Benefit

Each reader/writer has to be implemented only once, and then one can define a new DataFlowController by simply combining a reader and a writer.

Risk

Not really a risk, but implementation must be generic enough to be also compliant with stream data transfer, as in this case the data transfer does not consist of a simple read/write.

[DominikPinsel]

Hi @jimmarino,

in the IDS task you wrote

Impact

This may impact some Catena-X teams, e.g. the group working on the PRS prototype. We would need to liaise with them.

Do you know person from the PRS project I can contact regarding the IDS changes?

[DominikPinsel]

As discussed yesterday here a list of things I currently know of, where the EDC behaves differently than other non EDC connectors:

IDS-Multipart

• IDS Message sequence for data transfer implemented differently (async.)
• IDS Message sequence for contract negotiation implemented differently
• The ArtifactRequestMessage, that has normally no payload, got a payload with the DataAddress and Secret

Information Model

• All connectors that use a different information model version (always 1 supported at the same time)
• The following IDS objects may not be translated/transformed into the EDC domain, if they were created by a non EDC connector
  • Connector (Self-Description)
  • ResourceCatalog
  • Resource
  • Representation
  • ContactOffer
  • ContractAgreement
  • Prohibition
  • Permission
  • BinaryOperator
  • (maybe more)

[juliapampus]

The negotiation flow is also not as currently defined in IDS. It says both participants should send a ContractAgreementMessage.

[DominikPinsel]

@juliapampus Thanks :) I updated the list