feat: managing Asset via URN instead of ID

[StewartJingga]

Is your feature request related to a problem? Please describe.
I am planning to have a job/pipeline that will be getting and deleting a single asset.
Right now compass has those APIs to get and delete, but it is using asset's ID.

Problem is, the job will not have the asset's ID available when running, instead it can generate asset's URN.

Describe the solution you'd like

• New API to retrieve an asset using its URN
• New API to delete an asset using its URN

Describe alternatives you've considered

• Use GetAllAssets API and filter the results

[StewartJingga]

Approach 1

Modify current APIs to also accept URN.

1. GetAssetByID - [GET] /v1beta1/assets/{id}
2. DeleteAsset - [DELETE] /v1beta1/assets/{id}

Compass internally will use the given {id} from route parameter as both, asset's id or urn.

Approach 2

New API to use URN instead of ID

1. GetAssetByURN - [GET] /v1beta1/assets/urn/{urn}
2. DeleteAssetByURN - [DELETE] /v1beta1/assets/urn/{urn}

• Approach 1 is overall simpler and quicker to implement.
• Approach 2's HTTP endpoints cannot be RESTful as they all will conflict with current APIs using ID.

wdyt @bsushmith ?

[bsushmith]

@StewartJingga I'm of a slightly different opinion here.

Approach 1:

• We would alter the API definitions for this, and this can lead to confusion as it takes both values - either {id} or {urn}

Approach 2:

• The new APIs would be nice along with a deprecate tag on the APIs using {id}. We can remove them completely after a couple of versions.

Approach 2's HTTP endpoints cannot be RESTful as they all will conflict with current APIs using ID.

Can you elaborate on this?
Both[GET] /v1beta1/assets/{id} and [GET] /v1beta1/assets/urn/{urn} will not have endpoint conflicts. Please CMIIW here.

[StewartJingga]

Can you elaborate on this?
Both[GET] /v1beta1/assets/{id} and [GET] /v1beta1/assets/urn/{urn} will not have endpoint conflicts. Please CMIIW here.

@bsushmith They won't have conflict with the new endpoints that i'm proposing.
But those proposed endpoints are not RESTful in a way that is does not follow recommended convention of restful.

Another plus point when going with approach number 1 is, we can easily remove the old (using id) behaviour while still keeping it RESTful.

[bsushmith]

Makes sense. @StewartJingga +1

[ravisuhag]

@StewartJingga From an API point of view, approach 1 is better.

[sudo-suhas]

Notes from discussion with @StewartJingga:

For updating the existing endpoint in a backward compatible way, we need the endpoint to accept both ID and URN and return the asset. We can delegate the decision to the service layer or the repository layer.

Delegating to the service layer means that we check if the given ID is a UUID and call GetByID or GetByURN defined in the repository layer. This has the downside of not being able to fetch by URN if the URN is a UUID - if a user creates an asset with the URN as f742aa61-1100-445c-8d72-355a42e2fb59, GetAssetByID will always return 'not found'.

The repository layer could handle this by using id = $id or urn = $id. This however can match multiple rows if and when the user specifies a uuid as the URN for an asset and the uuid is the id for some other asset. This can theoretically be an attack vector.

Both scenarios are very unlikely but I feel it would be better to not return the asset as opposed to returning the wrong one, so leaning towards delegating to the service layer. Note that this limitation of the system would be temporary and would be addressed once we stop supporting get by ID.

Also wanted to call out some more implementation details before raising the PR:

• Since we are expecting URN to be unique going forward, we should add a unique index on the urn column in assets table. The existing index on urn, type, service can also be dropped.
• GetAssetByURN is internally used for upsert of an asset. We can remove this (not a breaking change since it is inside internal pkg) and switch over to GetAssetByID since it would support fetch by urn.
• Subsequently, the Find method on Repository would become unused and should be okay to remove. Note that this is a method on a publically exposed interface so it is technically a breaking change. Please call out if you think it is not okay to remove it.

edit: Technically, even adding a method GetByURN to the Repository interface is a breaking change.