[feat] add findRelationshipsV3 API (impl + unit tests) #504
Conversation
ybz1013
force-pushed
the
yanbzhao/find-relationships-v3
branch
from
22e2d21 to
edc45e1
Compare
| * @return A list of relationship records in SqlRow (col: source, destination, metadata, etc). | ||
| */ | ||
| @Nonnull | ||
| public <RELATIONSHIP extends RecordTemplate> List<SqlRow> findRelationshipsV2V3Core( |
There was a problem hiding this comment.
nit: does this need to be public?
| @Nonnull Class<ASSET_RELATIONSHIP> assetRelationshipClass, @Nullable Map<String, Object> wrapOptions, | ||
| int offset, int count) { | ||
| if (wrapOptions == null) { | ||
| throw new IllegalArgumentException("Please check your use of the findRelationshipsV3 method. wrapOptions cannot be null."); |
There was a problem hiding this comment.
If it can't be null why is it @Nullable?
There was a problem hiding this comment.
Let me make a change here to better guard this API.
This is more of a way to alert (potential) user if they dont know what this API does. If someone thinks they can pass in any thing as a wrapper and get a result, I would rather throw them an error here instead of letting it fail randomly later.
| private <ASSET_RELATIONSHIP extends RecordTemplate, RELATIONSHIP extends RecordTemplate> ASSET_RELATIONSHIP createAssetRelationshipWrapperForRelationship( | ||
| @Nonnull Class<RELATIONSHIP> relationshipType, @Nonnull Class<ASSET_RELATIONSHIP> assetRelationshipClass, | ||
| @Nonnull String metadata, @Nonnull String sourceUrn, @Nullable Map<String, Object> wrapOptions) { | ||
| // TODO: if other type of ASSET_RELATIONSHIP is needed, we need to distinguish it with wrapOptions and handles differently. |
There was a problem hiding this comment.
What do you mean by this? What "other type of ASSET_RELATIONSHIP" can there be? Can you share an example of a wrapOption?
There was a problem hiding this comment.
since AssetRelationship is an LI internal model, we need to pass it here, so why not make the API params more flexible.
say in the future, we have a AssetRelationshipV2, we can still reuse this API and change the code to handle differently.
the wrapOption can have something like { "relationship.version": "2.0" }, then we check and do parsing differently. Or just anything we may want to use in the future.
There was a problem hiding this comment.
my main concern is we cant keep making new versions of the same API, so adding a map for params can make it more flexible if we want to pass in something new later.
| public static final String RELATED_TO = "relatedTo"; | ||
| public static final String SOURCE = "source"; | ||
| public static final String METADATA = "metadata"; | ||
| public static final String ASSET_RELATIONSHIP_TYPE = "asset.relationship.type"; |
There was a problem hiding this comment.
what if we just call this something like relationship.return.type? make it more clear
Summary
To support Relationship 2.0 models, which doesnt have source in the relationship itself, we need a new API to return the source in a wrapper class along with the relationship2.0 model.
The internal use is AssetRelationship = source + relationship2.0 (destination only). I didn't want to hardcode the class here, so I left it extendable. User can pass it any RecordTemplate as wrapper class, but the implementation is only for AssetRelationship now.
I added a wrapOptions map in case we want to have other wrapper types in the future. Currently, it acts as a guard to prevent people who doesn't know this API to accidentally use it.
Testing Done
./gradlew build
unit tests with relationship1.0 and 2.0
Checklist