Datastore level will certify that the name is not already in use

Datastore level will assign the ID

Don't see that in datastore's AddCollection implementation. Is that something currently in review? Or I can add this line to datastore level.

#3528

We should also be setting the UpdatedAt and UpdatedBy fields

This creation could be abstracted out into standalone function that takes a request object and just fills out the fields since that operation will need to be done for Add, DryRun, Update

@keyallis @md2119
Identity has FullName and FriendlyName among other things. Is it correct to set the name to FullName here?

There is func for that UserFromContext(ctx context.Context)

You might have to do some digging to figure out a clean way of doing this (maybe generics!) but for future work we need to also include the Id field population in the object creation for Update requests

Ah yes, I will add an idGetter func() string to the arguments, and update can then pass func() {return request.GetId() } as that func.

About generics, I thought to do it using generics, but since we are calling functions GetName, GetDescription, etc on the passed obj, if I call them on generic type R, compilation would fail. The solution for that is to constrain the generic type using an interface. In this case the constraining interface would look like

type collectionRequest interface {
    *v1.CreateCollectionRequest | *v1.DryRunCollectionRequest | *v1.UpdateCollectionRequest
}

func [R collectionRequest] collectionRequestToCollection(ctx context, request R, isCreate bool) {...}

However, this fails to compile too since go 1.18 doesn't have a good support for union of structs within a constraining interface. Since *v1.CreateCollectionRequest and others are structs and not interfaces themselves, GetName, etc are still not accessible from collectionRequest.

The workaround for this is to then add those methods to the constraining interface as well, which makes the interface look like

type collectionRequest interface {
	*v1.CreateCollectionRequest

	GetName() string
	GetDescription() string
	GetResourceSelectors() []*storage.ResourceSelector
	GetEmbeddedCollectionIds() []string
}

This works, but then I still had to put those methods in the interface. so I just avoided the generics altogether for this case.

return error

because it implied from the context could not determine user identity seems more appropriate

valid resource selectors are non-empty

Added an error on empty resource selectors.

@keyallis

move this into collectionRequestToCollection(...)

implying getID wrt isCreate is confusing. Strongly recommend dropping this check and explicitly basing the condition on create and update workflow.

Ah, I merged this PR before noticing the comment. I will add it in my Delete collections service PR. Should I change to something like below?

if isCreate {...}
else {
    if getID == nil {
        return nil, err
    }
    collection.Id = getID()
}