feat(assets): Add create API with insert to fail on duplicates

[kotkar-pallavi]

Summary

• Adding a new method to create new assets for Create API.
• The current behavior is to create new assets as part of the upsert method. This new create method can be used to explicitly create assets.
• Create method will throw an exception if we try to insert records with a duplicate primary key instead of doing an update.

Testing Done

Local build and added unit tests

Checklist

• The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• Links to related issues (if applicable)
• Docs related to the changes have been added/updated (if applicable)

[jphui]

Solid PR, but a few high-level questions and potential gaps

1. (Ebean)LocalAccess is never directly invoked by the metadata-graph-assets service:

    assetModelPegasusInterop.getPegasusAspectsFromAsset(asset).stream()
       .forEach(aspect -> dao.add((URN) pegasusUrn, aspect, auditStamp, pegasusTrackingContext, pegasusParams));

It only has access to (Ebean)LocalDAO, through which LocalAccess is invoked:

EbeanLocalDAO.java:

  public <ASPECT extends RecordTemplate> void updateEntityTables(@Nonnull URN urn, @Nonnull Class<ASPECT> aspectClass) {
...
      ASPECT aspect = toRecordTemplate(aspectClass, result).orElse(null);
      _localAccess.add(urn, aspect, aspectClass, auditStamp, null, false);
...

I believe you will probably need to introduce a new set of methods inside EbeanLocalDao.java -- and / or perhaps BaseLocalDao.java -- in order to access the functionality here.

2. Design choice of adding a new method (done here in this PR) vs adding a flag for existing functionality

This is more of a question than a comment, but is there a reason that CREATE functionality is being added as a new method here as opposed to just adding a boolean flag or something to other existing add() methods?

At a cursory glance it seems like other than the SQL statement used, the logic flow is basically identical.

Maybe you've thought through future augmentations / feature-adds that are needed and it needs to be separate or something similar?

3. Ingestion callback requirements?

Do you know if Ingestion Callback will be needed for CREATE? If you aren't sure what this is, @ZihanLi58 / I / Yihong can explain the concept -- and Zihan can probably elaborate on the need -- but basically this is something supported by Normal Ingestion at the moment and is implemented at the DAO (not Access) level.

[ZihanLi58]

Can you share more requirement about the create API? basically
is it for createAsset or createAspect?
If createAsset, then I think you might need to accept more aspects in one request?
If createAspect, then we probably only want to fail the request when the urn and the aspect are all existing? If urn exist but aspect does not, we should still create it for user?

[ZihanLi58]

Solid PR, but a few high-level questions and potential gaps

1. (Ebean)LocalAccess is never directly invoked by the metadata-graph-assets service:

    assetModelPegasusInterop.getPegasusAspectsFromAsset(asset).stream()
       .forEach(aspect -> dao.add((URN) pegasusUrn, aspect, auditStamp, pegasusTrackingContext, pegasusParams));

It only has access to (Ebean)LocalDAO, through which LocalAccess is invoked:

EbeanLocalDAO.java:

  public <ASPECT extends RecordTemplate> void updateEntityTables(@Nonnull URN urn, @Nonnull Class<ASPECT> aspectClass) {
...
      ASPECT aspect = toRecordTemplate(aspectClass, result).orElse(null);
      _localAccess.add(urn, aspect, aspectClass, auditStamp, null, false);
...

I believe you will probably need to introduce a new set of methods inside EbeanLocalDao.java -- and / or perhaps BaseLocalDao.java -- in order to access the functionality here.

2. Design choice of adding a new method (done here in this PR) vs adding a flag for existing functionality

This is more of a question than a comment, but is there a reason that CREATE functionality is being added as a new method here as opposed to just adding a boolean flag or something to other existing add() methods?

At a cursory glance it seems like other than the SQL statement used, the logic flow is basically identical.

Maybe you've thought through future augmentations / feature-adds that are needed and it needs to be separate or something similar?

3. Ingestion callback requirements?

Do you know if Ingestion Callback will be needed for CREATE? If you aren't sure what this is, @ZihanLi58 / I / Yihong can explain the concept -- and Zihan can probably elaborate on the need -- but basically this is something supported by Normal Ingestion at the moment and is implemented at the DAO (not Access) level.

Based on the question I ask before, you can choose different design choice there. And to make sure the ingestion behavior is consistency, call back is required for create as well

[kotkar-pallavi]

@jphui

1. I have added the method definition and iml in BaseLocalDao.java. Thanks for pointing out.
2. The add method a lot of params not required for create. e.g.: the lambda function is not application on create functionality. I noticed the add method also contains a lot of legacy implementation leading to a very deep call stack. In order to keep the new impl clean and extendable for future improvements I decided to create a separate create method.
3. Added ingestion callbacks.

@ZihanLi58
The create API will be used to explicitly create a new asset. This is avoid the overloaded use of upsert method. We want to keep the interface similar to the existing upsert api, hence allowing create for single aspect instead of a list. Right now, upsert is done iteratively for each aspect.
The create method will throw error if a URN already exists. it does not matter if it contains any assets or not. The user is expected to use the update api to add more aspects and these exceptions will be defined in the asset service.
I added callbacks to create method and exposing it from the BaseLocalDao.java class.

please review. thanks!

[ZihanLi58]

@kotkar-pallavi Current update api actually accept asset as input, and it does the upsert for each aspect Iteratively. In your current approach, can the create user facing api take asset as input? If the asset contains multiple aspect, it seems will fail the request right as second create call for second aspect will fail since the first one already create the urn.

[jphui]

Other than Zihan's comment, lgtm from me

[jphui]

IIUC, CREATE will return the NEW value of the Asset after it (successfully) runs?

[kotkar-pallavi]

@kotkar-pallavi Current update api actually accept asset as input, and it does the upsert for each aspect Iteratively. In your current approach, can the create user facing api take asset as input? If the asset contains multiple aspect, it seems will fail the request right as second create call for second aspect will fail since the first one already create the urn.

@ZihanLi58 updated to create aspects with multiple aspects. it will be done in a single create statement that will throw exception if urn already exists. as discussed offline, doing a exists check followed by iterative create might lead to race condition. please re-review. thanks!

[yangyangv2]

Is the intention to replace :metadata with values (separated with comma) from multiple columns?

INSERT INTO table(urn, a_urn, column1, column2) VALUE (:urn, :a_urn, :metadata)

and

update.setParameter("metadata", "a, b");

I suspect if this is allowed, can you double check? and add an unit test for it

[kotkar-pallavi]

yes :metadata will have comma separated values. it is difficult to test and I tried adding unit test here: a59095e

from debugging with breakpoint, i verified the values and they look as follows:

 {"lastmodifiedon":"2025-02-23 23:13:29.767","aspect":{"value":"foo"},"lastmodifiedby":"urn:li:testActor:actor","canonicalName":"com.linkedin.testing.AspectFoo"},{"lastmodifiedon":"2025-02-23 23:13:29.767","aspect":{"value":"bar"},"lastmodifiedby":"urn:li:testActor:actor","canonicalName":"com.linkedin.testing.AspectBar"}

It has comma separated value for aspect foo and bar.

[ZihanLi58]

Logic looks good to me, but seems a lot useless code, can you do a clean up?