Override not editable fields during updates #200
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| package converter | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "go/ast" | ||
| "go/parser" | ||
| "go/token" | ||
| "os" | ||
| "reflect" | ||
| "regexp" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/opendatahub-io/model-registry/pkg/openapi" | ||
| ) | ||
|
|
||
| // visitor | ||
| type visitor struct { | ||
| t *testing.T | ||
| entities map[string]*oapiEntity | ||
| } | ||
|
|
||
| func newVisitor(t *testing.T, f *ast.File) visitor { | ||
| return visitor{ | ||
| t: t, | ||
| entities: map[string]*oapiEntity{ | ||
| "RegisteredModel": { | ||
| obj: openapi.RegisteredModel{}, | ||
| }, | ||
| "ModelVersion": { | ||
| obj: openapi.ModelVersion{}, | ||
| }, | ||
| "ModelArtifact": { | ||
| obj: openapi.ModelArtifact{}, | ||
| }, | ||
| "ServingEnvironment": { | ||
| obj: openapi.ServingEnvironment{}, | ||
| }, | ||
| "InferenceService": { | ||
| obj: openapi.InferenceService{}, | ||
| }, | ||
| "ServeModel": { | ||
| obj: openapi.ServeModel{}, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func (v *visitor) extractGroup(regex *regexp.Regexp, s string) string { | ||
| extracted := regex.FindStringSubmatch(s) | ||
| if len(extracted) != 2 { | ||
| v.t.Errorf("unable to extract groups from %s for %s", regex.String(), s) | ||
| } | ||
| // the first one is the wole matched string, the second one is the group | ||
| return extracted[1] | ||
| } | ||
|
|
||
| func (v *visitor) getEntity(name string) *oapiEntity { | ||
| val, ok := v.entities[name] | ||
| if !ok { | ||
| v.t.Errorf("openapi entity not found in the entities map: %s", name) | ||
| } | ||
| return val | ||
| } | ||
|
|
||
| func (v visitor) Visit(n ast.Node) ast.Visitor { | ||
| if n == nil { | ||
| return nil | ||
| } | ||
|
|
||
| switch d := n.(type) { | ||
| case *ast.InterfaceType: | ||
| for _, m := range d.Methods.List { | ||
| methodName := m.Names[0].Name | ||
|
|
||
| if converterMethodPattern.MatchString(methodName) { | ||
| entityName := v.extractGroup(converterMethodPattern, methodName) | ||
| entity := v.getEntity(entityName) | ||
| // there should be just one doc comment matching ignoreDirectivePattern | ||
| for _, c := range m.Doc.List { | ||
| if ignoreDirectivePattern.MatchString(c.Text) { | ||
| entity.notEditableFields = v.extractGroup(ignoreDirectivePattern, c.Text) | ||
| } | ||
| } | ||
| } else if overrideNotEditableMethodPattern.MatchString(methodName) { | ||
| entityName := v.extractGroup(overrideNotEditableMethodPattern, methodName) | ||
| entity := v.getEntity(entityName) | ||
| // there should be just one doc comment matching ignoreDirectivePattern | ||
| for _, c := range m.Doc.List { | ||
| if ignoreDirectivePattern.MatchString(c.Text) { | ||
| entity.ignoredFields = v.extractGroup(ignoreDirectivePattern, c.Text) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| v.checkEntities() | ||
| } | ||
| return v | ||
| } | ||
|
|
||
| // checkEntities check if all editable fields are listed in the goverter ignore directive of OverrideNotEditableFor | ||
| func (v *visitor) checkEntities() { | ||
| errorMsgs := map[string][]string{} | ||
| for k, v := range v.entities { | ||
| msgs := checkEntity(v) | ||
| if len(msgs) > 0 { | ||
| errorMsgs[k] = msgs | ||
| } | ||
| } | ||
|
|
||
| if len(errorMsgs) > 0 { | ||
| missingFieldsMsg := "" | ||
| for k, fields := range errorMsgs { | ||
| missingFieldsMsg += fmt.Sprintf("%s: %v\n", k, fields) | ||
| } | ||
| v.t.Errorf("missing fields to be ignored for OverrideNotEditableFor* goverter methods:\n%v", missingFieldsMsg) | ||
| } | ||
| } | ||
|
|
||
| // checkEntity check if there are missing fields to be ignored in the override method | ||
| func checkEntity(entity *oapiEntity) []string { | ||
| res := []string{} | ||
| objType := reflect.TypeOf(entity.obj) | ||
| for i := 0; i < objType.NumField(); i++ { | ||
|
There was a problem hiding this comment. I suspect there must be a way to make Field(s) iterable with There was a problem hiding this comment. TBH I couldn't find a different way to iterate over those fields, at least using the |
||
| field := objType.Field(i) | ||
| if !strings.Contains(entity.notEditableFields, field.Name) && !strings.Contains(entity.ignoredFields, field.Name) { | ||
| // check if the not editable field (first check) is not present in the ignored fields (second check) | ||
| // if this condition is true, we missed that field in the Override method ignore list | ||
| res = append(res, field.Name) | ||
| } | ||
| } | ||
| return res | ||
| } | ||
|
|
||
| // test | ||
|
|
||
| var converterMethodPattern *regexp.Regexp = regexp.MustCompile(`Convert(?P<entity>\w+)Update`) | ||
| var overrideNotEditableMethodPattern *regexp.Regexp = regexp.MustCompile(`OverrideNotEditableFor(?P<entity>\w+)`) | ||
| var ignoreDirectivePattern *regexp.Regexp = regexp.MustCompile(`// goverter:ignore (?P<fields>.+)`) | ||
|
|
||
| func TestOverrideNotEditableFields(t *testing.T) { | ||
| _ = setup(t) | ||
|
|
||
| fset := token.NewFileSet() // positions are relative to fset | ||
| wd, err := os.Getwd() | ||
| if err != nil { | ||
| t.Errorf("error getting current working directory") | ||
| } | ||
| filePath := fmt.Sprintf("%s/opeanpi_converter.go", wd) | ||
| f, _ := parser.ParseFile(fset, filePath, nil, parser.ParseComments) | ||
|
|
||
| v := newVisitor(t, f) | ||
| ast.Walk(v, f) | ||
| } | ||
|
|
||
| type oapiEntity struct { | ||
| obj any | ||
| notEditableFields string | ||
| ignoredFields string | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package converter | ||
|
|
||
| import "github.com/opendatahub-io/model-registry/pkg/openapi" | ||
|
|
||
| type OpenapiUpdateWrapper[ | ||
| M openapi.RegisteredModel | | ||
| openapi.ModelVersion | | ||
| openapi.ModelArtifact | | ||
| openapi.ServingEnvironment | | ||
| openapi.InferenceService | | ||
| openapi.ServeModel, | ||
| ] struct { | ||
| Existing *M | ||
| Update *M | ||
| } | ||
|
|
||
| func NewOpenapiUpdateWrapper[ | ||
| M openapi.RegisteredModel | | ||
| openapi.ModelVersion | | ||
| openapi.ModelArtifact | | ||
| openapi.ServingEnvironment | | ||
| openapi.InferenceService | | ||
| openapi.ServeModel, | ||
| ](existing *M, update *M) OpenapiUpdateWrapper[M] { | ||
| return OpenapiUpdateWrapper[M]{ | ||
| Existing: existing, | ||
| Update: update, | ||
| } | ||
| } | ||
|
|
||
| func InitRegisteredModelWithUpdate(source OpenapiUpdateWrapper[openapi.RegisteredModel]) openapi.RegisteredModel { | ||
| if source.Update != nil { | ||
| return *source.Update | ||
| } | ||
| return openapi.RegisteredModel{} | ||
| } | ||
|
|
||
| func InitModelVersionWithUpdate(source OpenapiUpdateWrapper[openapi.ModelVersion]) openapi.ModelVersion { | ||
| if source.Update != nil { | ||
| return *source.Update | ||
| } | ||
| return openapi.ModelVersion{} | ||
| } | ||
|
|
||
| func InitModelArtifactWithUpdate(source OpenapiUpdateWrapper[openapi.ModelArtifact]) openapi.ModelArtifact { | ||
| if source.Update != nil { | ||
| return *source.Update | ||
| } | ||
| return openapi.ModelArtifact{} | ||
| } | ||
|
|
||
| func InitServingEnvironmentWithUpdate(source OpenapiUpdateWrapper[openapi.ServingEnvironment]) openapi.ServingEnvironment { | ||
| if source.Update != nil { | ||
| return *source.Update | ||
| } | ||
| return openapi.ServingEnvironment{} | ||
| } | ||
|
|
||
| func InitInferenceServiceWithUpdate(source OpenapiUpdateWrapper[openapi.InferenceService]) openapi.InferenceService { | ||
| if source.Update != nil { | ||
| return *source.Update | ||
| } | ||
| return openapi.InferenceService{} | ||
| } | ||
|
|
||
| func InitServeModelWithUpdate(source OpenapiUpdateWrapper[openapi.ServeModel]) openapi.ServeModel { | ||
| if source.Update != nil { | ||
| return *source.Update | ||
| } | ||
| return openapi.ServeModel{} | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this extract all code comments pertaining to Methods. Nice, just as expected also from JavaParser AST I believe structure is analogous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah correct, it extracts all of them and then I just had to filter the one we are insterested in.