Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(artifact): using grpc error code #20

Merged
merged 2 commits into from
Jun 5, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/customerror/error.go
Original file line number Diff line number Diff line change
@@ -16,4 +16,7 @@ var (
ErrRateLimiting = errors.New("rate limiting")
// Batch size exceeded.
ErrExceedMaxBatchSize = errors.New("the batch size can not exceed 32")

// Invalid argument.
ErrInvalidArgument = errors.New("invalid argument")
)
6 changes: 6 additions & 0 deletions pkg/handler/errors.go
Original file line number Diff line number Diff line change
@@ -10,3 +10,9 @@ var ErrResourceID = errors.New("resource ID error")
var ErrSematicVersion = errors.New("not a legal version, should be the format vX.Y.Z or vX.Y.Z-identifiers")
var ErrUpdateMask = errors.New("update mask error")
var ErrConnectorNamespace = errors.New("can not use other's connector")

var ErrCreateKnowledgeBase = errors.New("failed to create knowledge base")
var ErrGetKnowledgeBases = errors.New("failed to get knowledge bases")
var ErrUpdateKnowledgeBase = errors.New("failed to update knowledge base")
var ErrDeleteKnowledgeBase = errors.New("failed to delete knowledge base")
var ErrInvalidKnowledgeBaseName = errors.New("invalid knowledge base Name")
47 changes: 21 additions & 26 deletions pkg/handler/knowledgebase.go
Original file line number Diff line number Diff line change
@@ -9,38 +9,34 @@ import (
"google.golang.org/grpc/metadata"

"github.com/instill-ai/artifact-backend/pkg/constant"
"github.com/instill-ai/artifact-backend/pkg/customerror"
"github.com/instill-ai/artifact-backend/pkg/repository"
artifactpb "github.com/instill-ai/protogen-go/artifact/artifact/v1alpha"
)

// todo: add error codes in the error message
const ErrorCreateKnowledgeBaseCode = 1000
const ErrorGetKnowledgeBasesCode = 1001
const ErrorUpdateKnowledgeBaseCode = 1002
const ErrorDeleteKnowledgeBaseCode = 1003

type ErrorMsg map[int]string
const ErrorCreateKnowledgeBaseMsg = "failed to create knowledge base: %v"
const ErrorListKnowledgeBasesMsg = "failed to get knowledge bases: %v "
const ErrorUpdateKnowledgeBaseMsg = "failed to update knowledge base: %v"
const ErrorDeleteKnowledgeBaseMsg = "failed to delete knowledge base: %v"

const ErrorCreateKnowledgeBaseMsg = "failed to create knowledge base: %w"
const ErrorListKnowledgeBasesMsg = "failed to get knowledge bases: %w "
const ErrorUpdateKnowledgeBaseMsg = "failed to update knowledge base: %w"
const ErrorDeleteKnowledgeBaseMsg = "failed to delete knowledge base: %w"

func (ph *PublicHandler) CreateKnowledgeBase(ctx context.Context, req *artifactpb.CreateKnowledgeBaseRequest) (*artifactpb.CreateKnowledgeBaseResponse, error) {

uid, err := getUserIDFromContext(ctx)
if err != nil {
msg := fmt.Sprintf("failed to get user id from header: %v", err)
return nil, fmt.Errorf(ErrorCreateKnowledgeBaseMsg, msg)
err := fmt.Errorf("failed to get user id from header: %v. err: %w", err, customerror.ErrUnauthenticated)
return nil, err
}
// check name if it is empty
if req.Name == "" {
msg := "name is required"
return nil, fmt.Errorf(ErrorCreateKnowledgeBaseMsg, msg)
err := fmt.Errorf("name is required. err: %w", ErrCheckRequiredFields)
return nil, err
}
nameOk := isValidName(req.Name)
if !nameOk {
msg := "name is invalid: " + req.Name
return nil, fmt.Errorf(ErrorCreateKnowledgeBaseMsg, msg)
msg := "name is invalid: %v. err: %w"
return nil, fmt.Errorf(msg, req.Name, customerror.ErrInvalidArgument)
}
res, err := ph.service.Repository.CreateKnowledgeBase(ctx,
repository.KnowledgeBase{
@@ -52,7 +48,7 @@ func (ph *PublicHandler) CreateKnowledgeBase(ctx context.Context, req *artifactp
},
)
if err != nil {
return nil, fmt.Errorf(fmt.Sprintf(ErrorCreateKnowledgeBaseMsg, err))
return nil, err
}
return &artifactpb.CreateKnowledgeBaseResponse{
Body: &artifactpb.KnowledgeBase{
@@ -101,16 +97,15 @@ func (ph *PublicHandler) ListKnowledgeBases(ctx context.Context, _ *artifactpb.L
func (ph *PublicHandler) UpdateKnowledgeBase(ctx context.Context, req *artifactpb.UpdateKnowledgeBaseRequest) (*artifactpb.UpdateKnowledgeBaseResponse, error) {
uid, err := getUserIDFromContext(ctx)
if err != nil {

return nil, fmt.Errorf(ErrorUpdateKnowledgeBaseMsg, err)
return nil, err
}
// check name if it is empty
if req.Name == "" {
return nil, fmt.Errorf(ErrorUpdateKnowledgeBaseMsg, "name is required")
return nil, fmt.Errorf("name is empty. err: %w", ErrCheckRequiredFields)
}
nameOk := isValidName(req.Name)
if !nameOk {
return nil, fmt.Errorf(ErrorUpdateKnowledgeBaseMsg, "name is invalid: "+req.Name)
return nil, fmt.Errorf("name: %s is invalid. err: %w", req.Name, customerror.ErrInvalidArgument)
}
// check if knowledge base exists
res, err := ph.service.Repository.UpdateKnowledgeBase(
@@ -125,7 +120,7 @@ func (ph *PublicHandler) UpdateKnowledgeBase(ctx context.Context, req *artifactp
},
)
if err != nil {
return nil, fmt.Errorf(ErrorUpdateKnowledgeBaseMsg, err)
return nil, err
}
// populate response
return &artifactpb.UpdateKnowledgeBaseResponse{
@@ -145,12 +140,12 @@ func (ph *PublicHandler) DeleteKnowledgeBase(ctx context.Context, req *artifactp
uid, err := getUserIDFromContext(ctx)
if err != nil {

return nil, fmt.Errorf(ErrorDeleteKnowledgeBaseMsg, err)
return nil, err
}
err = ph.service.Repository.DeleteKnowledgeBase(ctx, uid, req.Id)
if err != nil {

return nil, fmt.Errorf(ErrorDeleteKnowledgeBaseMsg, err)
return nil, err
}
return &artifactpb.DeleteKnowledgeBaseResponse{
ErrorMsg: "", StatusCode: 0,
@@ -161,13 +156,13 @@ func getUserIDFromContext(ctx context.Context) (string, error) {
if v, ok := md[strings.ToLower(constant.HeaderUserUIDKey)]; ok {
return v[0], nil
}
return "", fmt.Errorf("user id not found in context")
return "", fmt.Errorf("user id not found in context. err: %w", customerror.ErrUnauthenticated)
}

func isValidName(name string) bool {
name = strings.ToLower(name) // Convert the name to lowercase for case-insensitive matching
// Define the regular expression pattern
pattern := `^[a-z0-9 _-]+$`
pattern := `^[a-z0-9_-]+$`
// Compile the regular expression
re := regexp.MustCompile(pattern)
// Match the name against the regular expression
5 changes: 3 additions & 2 deletions pkg/middleware/interceptor.go
Original file line number Diff line number Diff line change
@@ -103,8 +103,9 @@ func AsGRPCError(err error) error {
errors.Is(err, handler.ErrResourceID),
errors.Is(err, handler.ErrSematicVersion),
errors.Is(err, handler.ErrConnectorNamespace),
errors.Is(err, handler.ErrUpdateMask):

errors.Is(err, handler.ErrUpdateMask),
errors.Is(err, handler.ErrInvalidKnowledgeBaseName),
errors.Is(err, customerror.ErrInvalidArgument):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Yougigun I saw this PR once merged but I would have liked to add the following comment. It is not a blocker, just for awareness.

I can't find the conversation (I think it happened on a PR like 1 month ago) but I proposed to @donch1989 using a single package to communicate errors through packages. These errors would be a bit more generic (not found, resource exists, invalid argument...) and the package generating the error would be responsible of wrapping a given error into the domain one. This has several advantages:

  • In the interceptor we'd reduce a lot the amount of errors and packages we manage. Right now when we add a feature that returns a given error, we need to remember to handle it in the interceptor.
  • We can implement a retry policy handler for Temporal workflows or other type of event handler without copying all the errors that middleware already handles.
  • Ideally we could define these errors in service, which acts as the domain layer. Sadly, the current dependency injections would cause dependency cycles. However, using a package to communicate errors will sanitize some of this dependencies (in the cases where the package calls a dependency through an interface but it still needs to import the implementation package to handle the error).

Copy link
Contributor Author

@Yougigun Yougigun Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of using a single error package. The reason I have a custom error is also to try and solve the dependency cycle. I will try to use only a single error package in the interceptor over the following iteration.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I have a custom error is also to try and solve the dependency cycle

Yes I'm feeling that pain, too. One problem we have is that, even if we communicate through interfaces, many times we define them in the implem package instead of in the caller / in a separate package, which produces the same dependencies as if we were using a struct directly.

code = codes.InvalidArgument
case
errors.Is(err, customerror.ErrNoPermission):
13 changes: 7 additions & 6 deletions pkg/repository/knowledgebase.go
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@ import (
"time"

"github.com/google/uuid"
"github.com/instill-ai/artifact-backend/pkg/customerror"
"gorm.io/gorm"
)

@@ -119,13 +120,13 @@ func (r *Repository) CreateKnowledgeBase(ctx context.Context, kb KnowledgeBase)
return nil, err
}
if nameExists {
return nil, fmt.Errorf("knowledge base name already exists")
return nil, fmt.Errorf("knowledge base name already exists. err: %w", customerror.ErrInvalidArgument)
}

// Create a new KnowledgeBase record
if err := r.db.WithContext(ctx).Create(&kb).Error; err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return nil, fmt.Errorf("knowledge base ID not found: %v", kb.KbID)
return nil, fmt.Errorf("knowledge base ID not found: %v, err:%w", kb.KbID, gorm.ErrRecordNotFound)
}
return nil, err
}
@@ -155,7 +156,7 @@ func (r *Repository) UpdateKnowledgeBase(ctx context.Context, uid string, kb Kno
// Find the KnowledgeBase record by ID
if err := r.db.WithContext(ctx).Where(conds, kb.KbID).First(&existingKB).Error; err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return nil, err
return nil, fmt.Errorf("knowledge base ID not found: %v", kb.KbID)
}
return nil, err
}
@@ -179,7 +180,7 @@ func (r *Repository) UpdateKnowledgeBase(ctx context.Context, uid string, kb Kno
var updatedKB KnowledgeBase
if err := r.db.WithContext(ctx).Where(conds, kb.KbID).First(&updatedKB).Error; err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return nil, fmt.Errorf("knowledge base ID not found: %v", kb.KbID)
return nil, fmt.Errorf("knowledge base ID not found: %v. err: %w", kb.KbID, gorm.ErrRecordNotFound)
}
return nil, err
}
@@ -195,7 +196,7 @@ func (r *Repository) DeleteKnowledgeBase(ctx context.Context, uid string, kbID s
conds := fmt.Sprintf("%v = ? AND %v IS NULL", KnowledgeBaseColumn.KbID, KnowledgeBaseColumn.DeleteTime)
if err := r.db.WithContext(ctx).First(&existingKB, conds, kbID).Error; err != nil {
if err == gorm.ErrRecordNotFound {
return err
return fmt.Errorf("knowledge base ID not found: %v. err: %w", kbID, gorm.ErrRecordNotFound)
}
return err
}
@@ -212,7 +213,7 @@ func (r *Repository) DeleteKnowledgeBase(ctx context.Context, uid string, kbID s
// Save the changes to mark the record as soft deleted
if err := r.db.WithContext(ctx).Save(&existingKB).Error; err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return fmt.Errorf("knowledge base ID not found: %v", existingKB.KbID)
return fmt.Errorf("knowledge base ID not found: %v. err: %w", existingKB.KbID, gorm.ErrRecordNotFound)
}
return err
}