Skip to content

Commit

Permalink
fix: allow updating emtpy description for a model (#177)
Browse files Browse the repository at this point in the history
Because

- allow user to update an empty description

This commit

- update `Description sql.NullString` in datamodel
  • Loading branch information
Phelan164 committed Sep 21, 2022
1 parent bd21492 commit 100ec84
Show file tree
Hide file tree
Showing 6 changed files with 124 additions and 46 deletions.
29 changes: 29 additions & 0 deletions integration-test/rest_update_model.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,35 @@ export function UpdateModel() {
r.json().model.update_time !== undefined,
});

payload = JSON.stringify({
"description": ""
})
check(http.patch(`${apiHost}/v1alpha/models/${model_id}`, payload, {
headers: genHeader(`application/json`)
}), {
[`PATCH /v1alpha/models/${model_id} task cls description empty response status`]: (r) =>
r.status === 200,
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.name`]: (r) =>
r.json().model.name === `models/${model_id}`,
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.uid`]: (r) =>
r.json().model.uid !== undefined,
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.id`]: (r) =>
r.json().model.id === model_id,
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.description`]: (r) =>
r.json().model.description === "",
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.model_definition`]: (r) =>
r.json().model.model_definition === model_def_name,
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.configuration`]: (r) =>
r.json().model.configuration !== undefined,
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.visibility`]: (r) =>
r.json().model.visibility === "VISIBILITY_PRIVATE",
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.owner`]: (r) =>
r.json().model.user === 'users/local-user',
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.create_time`]: (r) =>
r.json().model.create_time !== undefined,
[`PATCH /v1alpha/models/${model_id} task cls description empty response model.update_time`]: (r) =>
r.json().model.update_time !== undefined,
});
// clean up
check(http.request("DELETE", `${apiHost}/v1alpha/models/${model_id}`, null, {
headers: genHeader(`application/json`),
Expand Down
6 changes: 3 additions & 3 deletions internal/db/migration/000001_init.up.sql
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ CREATE TABLE IF NOT EXISTS "model_definition" (
"uid" UUID PRIMARY KEY,
"id" VARCHAR(63) NOT NULL,
"title" varchar(255) NOT NULL,
"documentation_url" VARCHAR(1024) NULL,
"icon" VARCHAR(1024) NULL,
"documentation_url" VARCHAR(1023) NULL,
"icon" VARCHAR(1023) NULL,
"release_stage" VALID_RELEASE_STAGE DEFAULT 'RELEASE_STAGE_UNSPECIFIED' NOT NULL,
"model_spec" JSONB NOT NULL,
"model_instance_spec" JSONB NOT NULL,
Expand All @@ -43,7 +43,7 @@ CREATE TABLE IF NOT EXISTS "model_definition" (
CREATE TABLE IF NOT EXISTS "model" (
"uid" UUID PRIMARY KEY,
"id" VARCHAR(63) NOT NULL,
"description" varchar(1024),
"description" varchar(1023) NULL,
"model_definition_uid" UUID NOT NULL,
"configuration" JSONB NULL,
"visibility" VALID_VISIBILITY DEFAULT 'VISIBILITY_PRIVATE' NOT NULL,
Expand Down
3 changes: 2 additions & 1 deletion pkg/datamodel/datamodel.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package datamodel

import (
"database/sql"
"database/sql/driver"
"encoding/json"
"errors"
Expand Down Expand Up @@ -75,7 +76,7 @@ type Model struct {
ID string `json:"id,omitempty"`

// Model description
Description string `json:"description,omitempty"`
Description sql.NullString

// Model definition
ModelDefinitionUid uuid.UUID `gorm:"model_definition_uid,omitempty"`
Expand Down
10 changes: 7 additions & 3 deletions pkg/handler/convert.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package handler

import (
"database/sql"
"encoding/json"
"fmt"
"strings"
Expand Down Expand Up @@ -47,8 +48,11 @@ func PBModelToDBModel(owner string, pbModel *modelPB.Model) *datamodel.Model {
return time.Time{}
}(),
},
ID: pbModel.GetId(),
Description: pbModel.GetDescription(),
ID: pbModel.GetId(),
Description: sql.NullString{
String: pbModel.GetDescription(),
Valid: true,
},
}
}

Expand All @@ -61,7 +65,7 @@ func DBModelToPBModel(modelDef *datamodel.ModelDefinition, dbModel *datamodel.Mo
Id: dbModel.ID,
CreateTime: timestamppb.New(dbModel.CreateTime),
UpdateTime: timestamppb.New(dbModel.UpdateTime),
Description: &dbModel.Description,
Description: &dbModel.Description.String,
ModelDefinition: fmt.Sprintf("model-definitions/%s", modelDef.ID),
Visibility: modelPB.Model_Visibility(dbModel.Visibility),
Configuration: func() *structpb.Struct {
Expand Down
49 changes: 34 additions & 15 deletions pkg/handler/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"bufio"
"bytes"
"context"
"database/sql"
"encoding/json"
"fmt"
"io"
Expand Down Expand Up @@ -359,9 +360,12 @@ func saveFile(stream modelPB.ModelService_CreateModelBinaryFileUploadServer) (ou
return "", &datamodel.Model{}, "", err
}
uploadedModel = datamodel.Model{
ID: fileData.Model.Id,
Visibility: datamodel.ModelVisibility(visibility),
Description: description,
ID: fileData.Model.Id,
Visibility: datamodel.ModelVisibility(visibility),
Description: sql.NullString{
String: description,
Valid: true,
},
Configuration: datatypes.JSON{},
Instances: []datamodel.ModelInstance{{
State: datamodel.ModelInstanceState(modelPB.ModelInstance_STATE_OFFLINE),
Expand Down Expand Up @@ -623,8 +627,11 @@ func HandleCreateModelByMultiPartFormData(w http.ResponseWriter, r *http.Request
ModelDefinitionUid: localModelDefinition.UID,
Owner: owner,
Visibility: datamodel.ModelVisibility(visibility),
Description: r.FormValue("description"),
Configuration: bModelConfig,
Description: sql.NullString{
String: r.FormValue("description"),
Valid: true,
},
Configuration: bModelConfig,
}

// Validate ModelDefinition JSON Schema
Expand Down Expand Up @@ -905,9 +912,12 @@ func createGitHubModel(h *handler, ctx context.Context, req *modelPB.CreateModel
ModelDefinitionUid: modelDefinition.UID,
Owner: owner,
Visibility: datamodel.ModelVisibility(visibility),
Description: description,
Configuration: bModelConfig,
Instances: []datamodel.ModelInstance{},
Description: sql.NullString{
String: description,
Valid: true,
},
Configuration: bModelConfig,
Instances: []datamodel.ModelInstance{},
}
for _, tag := range githubInfo.Tags {
instanceConfig := datamodel.GitHubModelInstanceConfiguration{
Expand Down Expand Up @@ -1101,9 +1111,12 @@ func createArtiVCModel(h *handler, ctx context.Context, req *modelPB.CreateModel
ModelDefinitionUid: modelDefinition.UID,
Owner: owner,
Visibility: datamodel.ModelVisibility(visibility),
Description: description,
Configuration: bModelConfig,
Instances: []datamodel.ModelInstance{},
Description: sql.NullString{
String: description,
Valid: true,
},
Configuration: bModelConfig,
Instances: []datamodel.ModelInstance{},
}
rdid, _ := uuid.NewV4()
tmpDir := fmt.Sprintf("./%s", rdid.String())
Expand Down Expand Up @@ -1321,9 +1334,12 @@ func createHuggingFaceModel(h *handler, ctx context.Context, req *modelPB.Create
ModelDefinitionUid: modelDefinition.UID,
Owner: owner,
Visibility: datamodel.ModelVisibility(visibility),
Description: description,
Configuration: bModelConfig,
Instances: []datamodel.ModelInstance{},
Description: sql.NullString{
String: description,
Valid: true,
},
Configuration: bModelConfig,
Instances: []datamodel.ModelInstance{},
}
rdid, _ := uuid.NewV4()
configTmpDir := fmt.Sprintf("/tmp/%s", rdid.String())
Expand Down Expand Up @@ -1675,7 +1691,10 @@ func (h *handler) UpdateModel(ctx context.Context, req *modelPB.UpdateModelReque
for _, field := range req.UpdateMask.Paths {
switch field {
case "description":
updateModel.Description = *req.Model.Description
updateModel.Description = sql.NullString{
String: *req.Model.Description,
Valid: true,
}
}
}
}
Expand Down
73 changes: 49 additions & 24 deletions pkg/service/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package service_test
//go:generate mockgen -destination mock_repository_test.go -package $GOPACKAGE github.com/instill-ai/model-backend/pkg/repository Repository

import (
"database/sql"
"fmt"
"testing"

Expand All @@ -26,9 +27,12 @@ func TestCreateModel(t *testing.T) {
ctrl := gomock.NewController(t)

newModel := datamodel.Model{
BaseDynamic: datamodel.BaseDynamic{UID: uuid.UUID{}},
ID: ID,
Description: "this is a test model",
BaseDynamic: datamodel.BaseDynamic{UID: uuid.UUID{}},
ID: ID,
Description: sql.NullString{
String: "this is a test model",
Valid: true,
},
ModelDefinitionUid: MODEL_DEFINITION,
Owner: OWNER,
}
Expand All @@ -55,9 +59,12 @@ func TestGetModelById(t *testing.T) {
ctrl := gomock.NewController(t)

newModel := datamodel.Model{
BaseDynamic: datamodel.BaseDynamic{UID: uuid.UUID{}},
ID: ID,
Description: "this is a test model",
BaseDynamic: datamodel.BaseDynamic{UID: uuid.UUID{}},
ID: ID,
Description: sql.NullString{
String: "this is a test model",
Valid: true,
},
ModelDefinitionUid: MODEL_DEFINITION,
Owner: OWNER,
}
Expand All @@ -80,9 +87,12 @@ func TestGetModelByUid(t *testing.T) {

uid := uuid.UUID{}
newModel := datamodel.Model{
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: "this is a test model",
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: sql.NullString{
String: "this is a test model",
Valid: true,
},
ModelDefinitionUid: MODEL_DEFINITION,
Owner: OWNER,
}
Expand Down Expand Up @@ -140,9 +150,12 @@ func TestRenameModel(t *testing.T) {

uid := uuid.UUID{}
newModel := datamodel.Model{
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: "this is a test model",
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: sql.NullString{
String: "this is a test model",
Valid: true,
},
ModelDefinitionUid: MODEL_DEFINITION,
Owner: OWNER,
}
Expand All @@ -163,9 +176,12 @@ func TestRenameModel(t *testing.T) {
EXPECT().
GetModelById(gomock.Eq(OWNER), "new ID", modelPB.View_VIEW_FULL).
Return(datamodel.Model{
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: "new ID",
Description: "this is a test model",
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: "new ID",
Description: sql.NullString{
String: "this is a test model",
Valid: true,
},
ModelDefinitionUid: MODEL_DEFINITION,
Owner: OWNER,
}, nil).
Expand All @@ -183,9 +199,12 @@ func TestPublishModel(t *testing.T) {

uid := uuid.UUID{}
newModel := datamodel.Model{
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: "this is a test model",
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: sql.NullString{
String: "this is a test model",
Valid: true,
},
ModelDefinitionUid: MODEL_DEFINITION,
Owner: OWNER,
}
Expand Down Expand Up @@ -221,9 +240,12 @@ func TestUnpublishModel(t *testing.T) {

uid := uuid.UUID{}
newModel := datamodel.Model{
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: "this is a test model",
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: sql.NullString{
String: "this is a test model",
Valid: true,
},
ModelDefinitionUid: MODEL_DEFINITION,
Owner: OWNER,
}
Expand Down Expand Up @@ -259,9 +281,12 @@ func TestUpdateModel(t *testing.T) {

uid := uuid.UUID{}
newModel := datamodel.Model{
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: "new description",
BaseDynamic: datamodel.BaseDynamic{UID: uid},
ID: ID,
Description: sql.NullString{
String: "this is a test model",
Valid: true,
},
ModelDefinitionUid: MODEL_DEFINITION,
Owner: OWNER,
}
Expand Down

0 comments on commit 100ec84

Please sign in to comment.