feat: implement backend support for tag colors

[haller33]

feat(devices): implement backend support for tag colors

• Add color field to device schema and update API endpoints to handle color
• Attribute with HEX validation. Existing tags default to empty.
• related issue Implement backend support for tag colors  #4350

[heiytor]

I also noticed that some of the newly added Tag methods don’t have tests. Could you implement them?

[heiytor]

You can use the ContainsFunc method from the slices package here.

[heiytor]

We usually check if MatchedCount is 0 and return an ErrNoDocuments. I believe it’s appropriate to do the same here to maintain consistent behavior.

[haller33]

I have checked, this not use of the MatchedCount has not been made by myself(and have not been intended to be changed), this could led for minimal changes in method behavior, they will remain consistent as to the rest of the other methods return value.

[heiytor]

Is it expected to have multiple tags with the same name for the same namespace? If not, you could use FindOne instead of Find.

Additionally, there’s a duplicate err != nil check here.

[haller33]

Actually this has been the first idea, but if there is nothing that you are looking for, so they return a error, that means that there is 'no data on collection' and this is a ambiguous error, for a single data or not existing on the collection, so, by not trying to handle a error as empty return data, is better to use the find and deal with empty array; the additional check will be changed

[heiytor]

IMHO, using Find here is an over-complicated solution and handling lists when we will always have at most one item is unnecessary. In the end, what you're doing is effectively the same as:

tag := new(models.Tags)
if err := s.db.Collection("tags").FindOne(ctx, bson.M{"name": tagName, "tenant_id": tenant}).Decode(tag); err != nil {
    return false, FromMongoError(err)
}

And when using the method, you can simply check:

if _, err := store.VerifyTagExist(ctx, tagName, tenantID); err != nil {
    if errors.Is(err, ErrNoDocuments) {
        // The tag does not exist here
    } else {
       // Another error happens
    }
}

[haller33]

Well, this is understandable, but, this kind of defies the idea of VerifyTagExist, the purpose of VerifyTagExist is to return a boolean value indicating whether a tag exists, while we could just use error handling instead, that would go against the method's intended design, the method was created to simply check and return true/false, not to handle business logic through error handling passing, is understandable that this is the pattern on the code base (for the ErrNoDocuments alone), but still is awkward to use the error as the intended return since there is already a boolean return value to provide that information

[heiytor]

I don't believe that returning an error instead of a boolean constitutes handling business logic. I maintain my opinion that using an array to handle a tag that will always contain at most one item is overly complicated.

If the method name no longer aligns with its original purpose, you could rename it to TagsGetOne and have it return the model itself instead of a boolean. In this case, you can simply check if the error is ErrNoDocuments and ignore the tag. The behavior would remain the same.

If the goal is just to check for the tag's existence, you could return something like FromMongoError(err) == ErrNoDocuments.

[heiytor]

However, if you doesn't agree with that, feel free to resolve the conversation.

[haller33]

Well, you can keep your opinion, but I must say that it's not 'overly complicated', since actually mixing an error with the intended information of a method is not a good pattern/idea (there is a clear separation of what is data, a boolean value, and what is an actual error). indeed, I also will keep the code on this way, if you wish so, since the behavior is the same, and you are just giving an opinion, but again, I'm not criticizing your opinion, just talking about the best interface and logic that fits VerifyTagExist.

[heiytor]

As discussed with @gustavosbarreto, please refactor this method to something like the following:

func (s *Store) TagGet(ctx context.Context, tagName, tenant string) (*models.Tags, error) {
    tag := new(models.Tags)
    if err := s.db.Collection("tags").FindOne(ctx, bson.M{"name": tagName, "tenant_id": tenant}).Decode(tag); err != nil {
        return nil, FromMongoError(err)
    }

    return tag, nil
}

When checking whether the tag exists or not, do:

if _, err := store.TagGet(ctx, tagName, tenantID); err != nil {
    if errors.Is(err, ErrNoDocuments) {
        // Tag not found
    } else {
       // Another error happens
    }
}

[heiytor]

Why do you need to delete and reinsert a list of tags instead of updating them?

[haller33]

This has to do with a way to simplify behavior in development as an atomic way and the old logic that no longer applies, since your observation, it really is better now to just update, thank you

[gustavosbarreto]

The err variable is not updated within the loop

[gustavosbarreto]

The Decode method is never called to populate the tag variable with the data retrieved from db.

[gustavosbarreto]

This check is unnecessary since the presence of a document inherently confirms the tag's existence.

[gustavosbarreto]

The cursor is not explicitly closed, which could lead to resource leaks.