markylaing · 2024-04-05T11:29:58Z

Opening as a draft to get some feedback.

The type of entity.Type has been changed to an interface.
A new type has been created for each shared entity type that implements the interface. These types are empty structs for the purpose of embedding them into the database type. The types are defined as entity.Instance, entity.Network and so on. A variable is also defined for each entity type for ease of use (e.g. var TypeInstance = Instance{}).
Because each entity requires different arguments for creating a URL, these methods are defined on the concrete types and this is not part of the interface.
In the db/cluster package, the EntityType type is now a struct which embeds an entityType interface, the entityType interface embeds (shared/entity).Type and has methods for returning SQL for each entity type.
A consequence of using interfaces for entity types is that we need to be more careful when checking for equality. We need to always compare them by the Name() function (e.g. entityType1.Name() == entityType2.Name(). I've added an Equals method to the shared/entity package for this.

In order to add a new entity type, one needs to implement the interface in the shared package following the same pattern, and add the entity type to the list of all entity types in that package. Afterwards, the interface in the database package must be implemented (with the shared entity type being embedded in the database type) and this type must also be added to the list in the database package.

Closes #12928

markylaing · 2024-04-10T08:47:42Z

@tomponline this is ready for review when you have some time. It will likely conflict with #13298 so I'll need to rebase when that's merged.

markylaing · 2024-04-10T12:58:27Z

Rebased and ready for review. Thanks.

markylaing · 2024-04-11T12:02:12Z

Ready for round two :)

markylaing · 2024-04-11T13:51:44Z

I've just pushed again as I realised when working on #13072 that I had missed some equality checks that should now be using the entity.Equals method.

tomponline · 2024-04-12T10:17:34Z

+	if len(entityTypesFilter) == 0 && projectName == "" {
+		for _, dbEntityType := range entityTypes {
+			stmt := dbEntityType.AllURLs()
+			if stmt != "" {


Should an empty stmt be an error condition?

Hmmm... for this one perhaps but not for URLsByProject for example, as not all entities are project specific.

The implementation of the "server" entityType in the cluster package returns an empty string for AllURLs because there is no database query should be run for it (the caller should know that it's /1.0).

Additionally, remove the "nolint" directive as this function is now in use. Signed-off-by: Mark Laing <mark.laing@canonical.com>

Signed-off-by: Mark Laing <mark.laing@canonical.com>

The entity deletion triggers are now composed of a trigger name and the SQL for creating it. Signed-off-by: Mark Laing <mark.laing@canonical.com>

Create a map of `entity.TypeName` to `[]Entitlement` for entitlement validation. Signed-off-by: Mark Laing <mark.laing@canonical.com>

Signed-off-by: Mark Laing <mark.laing@canonical.com>

…ument. Signed-off-by: Mark Laing <mark.laing@canonical.com>

Signed-off-by: Mark Laing <mark.laing@canonical.com>

markylaing · 2024-06-13T08:45:05Z

@tomponline have rebased

@tomponline

@tomponline @hamistao this is a much more concise entity type refactor that I think works much better. (Thanks Tom for the idea of keeping the types as they are, but storing them in a map the returns a struct/interface with relevant fields/methods). With this change, I think it should be easy to add the URL prefix classification by: 1. Adding a `PrefixMatch() []string` method to `typeImpl` that returns a slice of URL prefixes to match the entity type against. 2. Adding a function `MatchURLPrefix` which iterates over the `entityType` map, and iterates over the `PrefixMatch()` array for each `typeImpl`, then returns the entity type if `strings.HasPrefix(u.Path, prefix)`. Closes #13262 Closes #12928

markylaing self-assigned this Apr 5, 2024

markylaing force-pushed the entity-type-refactor branch 5 times, most recently from 829e518 to c62358f Compare April 10, 2024 07:35

markylaing mentioned this pull request Apr 10, 2024

lxd: GET /1.0/warnings should use the can_view_warnings entitlement. #13298

Merged

markylaing requested review from gabrielmougard and roosterfish April 10, 2024 08:46

markylaing requested a review from tomponline as a code owner April 10, 2024 08:46

markylaing force-pushed the entity-type-refactor branch from c62358f to 550d572 Compare April 10, 2024 12:58

tomponline reviewed Apr 10, 2024

View reviewed changes

Comment thread shared/entity/type.go

tomponline reviewed Apr 10, 2024

View reviewed changes

Comment thread shared/entity/type.go Outdated

tomponline reviewed Apr 10, 2024

View reviewed changes

Comment thread shared/entity/type.go

tomponline reviewed Apr 10, 2024

View reviewed changes

Comment thread shared/entity/type.go Outdated

markylaing force-pushed the entity-type-refactor branch from 550d572 to 7ee82c6 Compare April 11, 2024 11:57

markylaing requested a review from tomponline April 11, 2024 12:02

markylaing force-pushed the entity-type-refactor branch from 7ee82c6 to ccf8b55 Compare April 11, 2024 13:50

markylaing mentioned this pull request Apr 11, 2024

Auth: Add project query parameter to URLs in authorizer #13317

Merged