X-Constraint List value validation fix

Rangababu-R commented 2 years ago

https://github.com/open-traffic-generator/openapiart/issues/367

ashutshkumr commented 2 years ago

Why do we store warnings as a string and not []string ? When we find something deprecated / under-review, we could just append it (instead of re-assigning) right ?

func (api *api) deprecated(message string) {
    api.warnings = message
    fmt.Printf("warning: %s\n", message)
}

We might benefit from having AddWarning() or AppendWarning() method, which will internally log the warning lines when they're being added.

ashutshkumr commented 2 years ago

Following line should be moved to its own function, maybe LogWarning.

fmt.Printf("warning: %s\n", message)

ashutshkumr commented 2 years ago

Why don't we implement Close on type api struct ? I noticed out of 8 methods, only 7 are implemented. That implies api doesn't completely implement Api interface.

It's probably ok to have a dummy implementation or an implementation that just prints warning that Close is not implemented.

ashutshkumr commented 2 years ago

Can we do something about sanity naming ? Curious why the proto files are not generated with name openapiartpb (derived from the package name itself).

I'm mentioning this because some users raised concerns about the naming of these files itself (they blindly copied the contents of artifact.py).

Another concern, why openapiart_class takes so many YAMLs as input ? Isn't there a single entry point ?

ashutshkumr commented 2 years ago

I think it's bad UX to have a type <package>.<name>Api instead of <package>.Api for objects returned from <package>.NewApi()

There's really no incentive for users to use base API class. So I would propose we rename base api class to BaseApi or something and rename actual API class to Api.

This might cause breaking change, and hence for some time, we might want to keep an alias.

// a should be of type openapiart.Api
a := openapiart.NewApi()

// if a func accepts and object of type openapiart.OpenapiArt, it should also be able to accept object of type openapiart.Api (i.e. one should be an alias of another)

ashutshkumr commented 2 years ago

I’m getting following output for a custom test I wrote.

func TestPrefixConfigCustom(t *testing.T) {
    api := openapiart.NewApi()
    c1 := api.NewPrefixConfig()

    c, e := c1.ToJson()
    fmt.Println("errors:", e)
    fmt.Println("config:", c)
}

go test -v -run "TestPrefixConfigCustom"
2022/10/10 10:02:39 MockGrpcServer: Server started and listening on address [::]:40051
2022/10/10 10:02:39 MockGrpcServer: Server subscribed with gRPC Protocol Service
2022/10/10 10:02:39 Generated Http Server serving incoming HTTP requests on  127.0.0.1:50051
=== RUN   TestPrefixConfigCustom
errors: RequiredObject is required field on interface PrefixConfig
A is required field on interface PrefixConfig
validation errors
config: 
--- PASS: TestPrefixConfigCustom (0.00s)
PASS
ok      github.com/open-traffic-generator/openapiart/pkg        0.004s

I expect to see 4 errors based on the model, but I'm only getting 2.

    Prefix.Config:
      type: object
      required: [a, b, c, required_object]

Not sure why this isn’t caught in the openapiart UTs.

EDIT: It's a long standing known issue. We don't really validate required fields with primitive non-string type.

ashutshkumr commented 2 years ago

There are lots of identifiers with underscore in their name (e.g. under_review, etc). Would suggest renaming those to use camelCase convention (which is idiomatic in Go).

ashutshkumr commented 2 years ago

Would suggest renaming following

type validation struct {
    validationErrors []string
    warnings         []string
    constraints      map[string]map[string]Constraints
    rootObj          rootObject
    temp             []rootObject
    resolve          bool
}

to

type typeMeta struct {
    errors              []string
    warnings         []string
    constraints      map[string]map[string]Constraints
    rootType         rootType
        // what is temp for ?
    temp             []rootObject
       // resolve what ?
    resolve          bool
}

In general, would like to replace the term validation wherever it's more appropriate to call it typeMeta and replace object with type.

ashutshkumr commented 2 years ago

From https://go.dev/doc/effective_go#interface-names: By convention, one-method interfaces are named by the method name plus an -er suffix or similar modification to construct an agent noun: Reader, Writer, Formatter, CloseNotifier etc.

Hence, about Constraints interface:

It should be called ValueGetter instead
ValueOf() should be renamed to StringValue() and return only string (since we only need string members for such validation)
Implementation of StringValue() should use switch-case instead of if-else, and it should not include non-string members
We could add more getters for primitive types if needed in future

The reason I mentioned this is because this code snippet is slow and dangerous.

        for _, object := range strukt {
            intf := object.ValueOf(obj_[1])
            if intf == nil {
                continue
            }
            if value == fmt.Sprintf("%v", intf) {
                found = true
                break
            }
        }

Let's also make sure that x-constraints allows constraining only string values (and validate as part of openapiart) - this needs to be a separate issue/PR if not already supported.

ashutshkumr commented 2 years ago

We should rename following for more consistency:

*Holder -> *Wrapper
Msg() -> Proto()
SetMsg() -> SetProto()
obj -> proto
func (obj *prefixConfig) FromYaml -> func (o *prefixConfig) FromYaml - since it's idomatic to use single letter in method definition

ashutshkumr commented 2 years ago

Has* methods have incorrect documentation.

// Space1 returns a int32
// description is TBD
func (obj *prefixConfig) HasSpace1() bool {
    return obj.obj.Space_1 != nil
}

Moreover, a int32 is grammatically incorrect.

ashutshkumr commented 2 years ago

We do not print names of attributes which are under review. e.g. in this case A. Also, curious why we see duplicate prints (is it because of usage of Set* and ToJson ?). Shouldn't we just print it during ToJson ?

func TestPrefixConfigCustom(t *testing.T) {
    api := openapiart.NewApi()
    c1 := api.NewPrefixConfig()
    c1.RequiredObject()
    c1.SetA("hello")
    c1.SetB(5)

    c, e := c1.ToJson()
    fmt.Println("errors:", e)
    fmt.Println("config:", c)
}

Output

2022/10/10 10:14:21 MockGrpcServer: Server started and listening on address [::]:40051
2022/10/10 10:14:21 MockGrpcServer: Server subscribed with gRPC Protocol Service
2022/10/10 10:14:21 Generated Http Server serving incoming HTTP requests on  127.0.0.1:50051
=== RUN   TestPrefixConfigCustom
warning: property under review
warning: Property b is being deprecated from the sdk version x.x.x
and property x shall be used instead
warning: property under review
warning: Property b is being deprecated from the sdk version x.x.x
and property x shall be used instead
errors: <nil>
config: {
  "required_object": {},
  "response": "status_200",
  "a": "hello",
  "b": 5,
  "h": true
}

ashutshkumr commented 2 years ago

openapiart should raise an error if default is provided for a required field.

ashutshkumr commented 2 years ago

I see lots of commented out code before return statement, can we please get rid of those ? e.g.

func (api *openapiartApi) NewGetConfigResponse() GetConfigResponse {
    vObj := validation{}
    obj := newGetConfigResponse(&vObj)
    vObj.rootObj = obj
    vObj.resolve = true
    return obj
    // return NewGetConfigResponse()
}

Rangababu-R commented 2 years ago

Why do we store warnings as a string and not []string ? When we find something deprecated / under-review, we could just append it (instead of re-assigning) right ?
func (api *api) deprecated(message string) {
  api.warnings = message
  fmt.Printf("warning: %s\n", message)
}
We might benefit from having AddWarning() or AppendWarning() method, which will internally log the warning lines when they're being added.

we had discussed this before and concluded to print the warning

Rangababu-R commented 2 years ago

Why don't we implement Close on type api struct ? I noticed out of 8 methods, only 7 are implemented. That implies api doesn't completely implement Api interface.

It's probably ok to have a dummy implementation or an implementation that just prints warning that Close is not implemented.

this is not related to x-constraints, unique and deprecate, please create an issue will be addressed in separate PR

Rangababu-R commented 2 years ago

Can we do something about sanity naming ? Curious why the proto files are not generated with name openapiartpb (derived from the package name itself).

I'm mentioning this because some users raised concerns about the naming of these files itself (they blindly copied the contents of artifact.py).

Another concern, why openapiart_class takes so many YAMLs as input ? Isn't there a single entry point ?

this is not related to x-constraints, unique and deprecate, please create an issue will be addressed in separate PR

Rangababu-R commented 2 years ago

I think it's bad UX to have a type <package>.<name>Api instead of <package>.Api for objects returned from <package>.NewApi()

There's really no incentive for users to use base API class. So I would propose we rename base api class to BaseApi or something and rename actual API class to Api.

This might cause breaking change, and hence for some time, we might want to keep an alias.
// a should be of type openapiart.Api
a := openapiart.NewApi()

// if a func accepts and object of type openapiart.OpenapiArt, it should also be able to accept object of type openapiart.Api (i.e. one should be an alias of another)

this is not related to x-constraints, unique and deprecate, please create an issue will be addressed in separate PR

Rangababu-R commented 2 years ago

We should rename following for more consistency:

* `*Holder` -> `*Wrapper`

* `Msg()` -> `Proto()`

* `SetMsg()` -> `SetProto()`

* `obj` -> `proto`

* `func (obj *prefixConfig) FromYaml` -> `func (o *prefixConfig) FromYaml` -  since it's idomatic to use single letter in method definition

this is not related to x-constraints, unique and deprecate, please create an issue will be addressed in separate PR

Rangababu-R commented 2 years ago

Has* methods have incorrect documentation.
// Space1 returns a int32
// description is TBD
func (obj *prefixConfig) HasSpace1() bool {
  return obj.obj.Space_1 != nil
}
Moreover, a int32 is grammatically incorrect.

this is not related to x-constraints, unique and deprecate, please create an issue will be addressed in separate PR

Rangababu-R commented 2 years ago

openapiart should raise an error if default is provided for a required field.

this is not related to x-constraints, unique and deprecate, please create an issue will be addressed in separate PR

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert when merging a4362122a8f11f29a9f0a8c263660976d3182ed3 into 7b70e6d82a185e57e50f024f75c88ef9238842bd - view on LGTM.com

new alerts:

1 for Unused local variable

ashutshkumr commented 10 months ago

Latest changes covered by https://github.com/open-traffic-generator/openapiart/pull/453

open-traffic-generator / openapiart

X-Constraint List value validation fix #366