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

Redesigned ConnectedAssetUniverse instantiation #7534

Merged
merged 12 commits into from
Mar 23, 2023

Conversation

LeeVi3w
Copy link
Contributor

@LeeVi3w LeeVi3w commented Mar 16, 2023

Description

Changed the instantiation method of ConnectedAssetUniverse from public constructors to static factory methods which return new objects.

This change was made to allow the use of the AssetUniverse super constructor with parameters as to set the missing properties reported in issue #7505. This decision was taken due to the fact that the super constructor needed data which was retrieved after its call inside the ConnectedAssetUniverse constructor.

The static factory methods introduced (one for each existing constructor) have the advantage of being able to process the needed data beforehand and then passing it to the ConnectedAssetUniverse, which in turn can call the super constructor that populates the missing properties with the given data. The newly created ConnectedAssetUniverse object is then returned.

These static factory methods are called create, and they make use of only one (private) ConnectedAssetUniverse constructor which takes an AssetResponse object and extracts the Asset bean containing the needed properties.

I have chosen this approach to avoid having to separate the data gathering logic from the class itself and to avoid having to set the missing fields in a dedicated function when there already exists a super constructor that deals with this.

Related Issue(s)

#7505 [BUG] ConnectedAssetUniverse not gathering all asset properties

Testing

The testing has been manually done using an instance of ConnectedAssetClient inside a Java application.

@LeeVi3w LeeVi3w force-pushed the connected-asset-missing-props branch from 306ee83 to 65e569c Compare March 20, 2023 10:52
@LeeVi3w LeeVi3w force-pushed the connected-asset-missing-props branch from 65e569c to 392985b Compare March 20, 2023 10:59
Copy link
Contributor

@mandy-chessell mandy-chessell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - just a few layout issues

@mandy-chessell
Copy link
Contributor

Need to correct the signing of your commits Click on the "DCO" details

@LeeVi3w LeeVi3w force-pushed the connected-asset-missing-props branch 2 times, most recently from b90e6e7 to 6f2fe7d Compare March 21, 2023 08:53
LeeVi3w and others added 3 commits March 21, 2023 10:55
Signed-off-by: Liviu Enache <[email protected]>
Signed-off-by: Liviu Enache <[email protected]>
@LeeVi3w LeeVi3w force-pushed the connected-asset-missing-props branch from 6f2fe7d to a004c18 Compare March 21, 2023 08:55
@planetf1 planetf1 mentioned this pull request Mar 23, 2023
33 tasks
@lpalashevski lpalashevski merged commit ff4b4b9 into odpi:main Mar 23, 2023
@LeeVi3w LeeVi3w deleted the connected-asset-missing-props branch March 23, 2023 18:29
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