Resolver: Place snippet marker more accuratly by AlonaKaplan · Pull Request #82 · nmstate-archive/nmpolicy

AlonaKaplan · 2022-01-17T11:33:15Z

The PR contains 3 commits-

Place snippet marker on correct part of a Ternary operation
Place snippet marker on correct key of a path
Support optional keys on any part of the path (continues PR resolver, eqfilter: Support optional fields #75, which allowed optional keys on the last key of the path).

netlify · 2022-01-17T11:33:22Z

✔️ Deploy Preview for nmpolicy ready!

🔨 Explore the source changes: d8e17a3

🔍 Inspect the deploy log: https://app.netlify.com/sites/nmpolicy/deploys/61e95512ee92bd0007a9f0cd

😎 Browse the preview: https://deploy-preview-82--nmpolicy.netlify.app

qinqon

We can prevent changing all the receivers to pointer receivers also updateError is not needed.

qinqon · 2022-01-19T08:23:29Z

nmpolicy/internal/resolver/errors.go

+)
+
+type PathError struct {
+	err       error


Can we name this inner like https://github.com/nmstate/nmpolicy/blob/main/nmpolicy/internal/parser/errors.go#L29-L31

Also we have to implement Unwrap function to support error unwrapping
https://github.com/nmstate/nmpolicy/blob/main/nmpolicy/internal/parser/errors.go#L29-L31

Done. Although I think the Unwrap is redundant in this stage, it is not used.

nmpolicy/internal/resolver/errors.go

qinqon · 2022-01-19T08:28:02Z

nmpolicy/internal/resolver/errors.go

+func pathError(currentStepNode ast.Node, format string, a ...interface{}) PathError {
+	return wrapWithPathError(currentStepNode, fmt.Errorf(format, a...))
+}
+
+func wrapWithPathError(currentStepNode ast.Node, err error) PathError {
+	return PathError{err: fmt.Errorf("invalid path: %v", err), errorNode: currentStepNode}


The invalid path has to be a prefix used at both pathError and wrapWithPathError

It is, pathError calls wrapWithPathError.

qinqon · 2022-01-19T09:43:48Z

nmpolicy/internal/resolver/path.go


 type pathVisitor struct {
 	path              []ast.Node
+	currentStep       ast.Node


This can be an int with the path index so we don't have to change v pathVisitor to v* pathVisitor only because the struct has too much stuff, and alternative is to make currentStep a pointer.

changed currentStep to be a pointer.

nmpolicy/internal/resolver/resolver.go

qinqon · 2022-01-19T09:56:14Z

nmpolicy/internal/resolver/resolver.go

 	r.currentNode = &(*operator)[2]
 	value, err := r.resolveStringOrCaptureEntryPath()
 	if err != nil {
+		r.updatePathError(err)


The error "downcastig" to PathError to discover the node has to be done at https://github.com/nmstate/nmpolicy/blob/6ba34e92110dbbcb092a3aff5e706afa09306f36/nmpolicy/internal/resolver/resolver.go#L230-L238

…ration Signed-off-by: Alona Kaplan <alkaplan@redhat.com>

qinqon

Let's not change resolver receiver if it's not needed

qinqon · 2022-01-20T10:24:52Z

nmpolicy/internal/resolver/resolver.go

+
+	var pathError PathError
+	if errors.As(err, &pathError) {
+		r.currentNode = &pathError.errorNode


You don't need to change resolver state also, the resolver is "by value" at the reciver let's just do the following it will be less confusing:

errorNode := r.currentNode if errors.As(err, &pathError) { errorNode = &pathError.errorNode } if errorNode == nil || r.currentExpression == nil { return err } return expression.WrapError(err, *r.currentExpression, errorNode.Meta.Position)

Since it is "by value` it can be changed and it will effect only the current method.
But I"m ok with both versions, so done.

qinqon · 2022-01-20T10:26:37Z

nmpolicy/internal/resolver/errors.go

+
+type PathError struct {
+	inner     error
+	errorNode ast.Node


This should be a pointer.

To detect if we are missing setting it (by a bug or the like) is better if we have a pointer if we don't it's more difficult.

We are using like this right now

if errors.As(err, &pathError) { errorNode = &pathError.errorNode }

So even if it's not set it will set it to default errorNode with have position 0 wich is wrong, so better be explicit about errorNode not set using a pointer.

qinqon · 2022-01-20T11:23:44Z

nmpolicy/internal/resolver/resolver.go

+		errorNode = &pathError.errorNode
+	}
+
 	if r.currentNode == nil || r.currentExpression == nil {


Suggested change

if r.currentNode == nil || r.currentExpression == nil {

if errorNode == nil || r.currentExpression == nil {

Signed-off-by: Alona Kaplan <alkaplan@redhat.com>

This commit continues PR nmstate-archive#75. In case the path contains fields that don't exist on the input state, the resolver shouldn't fail, just return an empty result. Signed-off-by: Alona Kaplan <alkaplan@redhat.com>

AlonaKaplan force-pushed the pathError branch from b035a7b to f7fe7a1 Compare January 17, 2022 11:47

qinqon self-requested a review January 17, 2022 11:48

AlonaKaplan force-pushed the pathError branch from f7fe7a1 to 6476cfe Compare January 17, 2022 12:01

qinqon suggested changes Jan 19, 2022

View reviewed changes

resolver error: Place snippet marker on correct part of a Ternary ope…

e843411

…ration Signed-off-by: Alona Kaplan <alkaplan@redhat.com>

AlonaKaplan force-pushed the pathError branch from 6476cfe to 3af5440 Compare January 20, 2022 09:49

qinqon suggested changes Jan 20, 2022

View reviewed changes

AlonaKaplan force-pushed the pathError branch from 3af5440 to fb18beb Compare January 20, 2022 10:40

qinqon reviewed Jan 20, 2022

View reviewed changes

AlonaKaplan force-pushed the pathError branch from fb18beb to 883473b Compare January 20, 2022 11:53

AlonaKaplan added 2 commits January 20, 2022 14:26

resolver error: Place snippet marker on correct part of a path

73d8e32

Signed-off-by: Alona Kaplan <alkaplan@redhat.com>

resolver: support optional keys on any part of the path

d8e17a3

This commit continues PR nmstate-archive#75. In case the path contains fields that don't exist on the input state, the resolver shouldn't fail, just return an empty result. Signed-off-by: Alona Kaplan <alkaplan@redhat.com>

AlonaKaplan force-pushed the pathError branch from 883473b to d8e17a3 Compare January 20, 2022 12:26

qinqon approved these changes Jan 20, 2022

View reviewed changes

qinqon merged commit 51221c1 into nmstate-archive:main Jan 20, 2022

	if r.currentNode == nil \|\| r.currentExpression == nil {
	if errorNode == nil \|\| r.currentExpression == nil {

Conversation

AlonaKaplan commented Jan 17, 2022

Uh oh!

netlify bot commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qinqon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinqon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinqon Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

netlify bot commented Jan 17, 2022 •

edited

Loading

qinqon Jan 20, 2022 •

edited

Loading