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.

Sign up for GitHub

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

Jump to bottom

Add warn when input attributes is missing #4416

Merged
merged 1 commit into from Mar 24, 2022
Merged

Add warn when input attributes is missing #4416

merged 1 commit into from Mar 24, 2022

Conversation

aflmp
Copy link
Contributor

@aflmp aflmp commented Mar 8, 2022

Fixes: #4386

@aflmp
Copy link
Contributor Author

aflmp commented Mar 8, 2022

Hello @anderseknert, does the approach look ok to you? If yes, I can work on adding/fixing tests.

anderseknert
anderseknert requested changes Mar 9, 2022
View reviewed changes
Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

Thanks! Left a comment on the URL as I think we might want to skip that. Looks like several tests need to be updated to work with this addition though, and perhaps you'd want to add a test or two on your own, unless this is already well covered by those that are there already (I think that might be the case).

server/types/types.go Outdated
const CodeAPIUsageWarn = "api_usage_warning"

// Warning Messages
const MsgInputKeyMissing = "'input' key missing: see https://www.openpolicyagent.org/docs/latest/#4-try-opa-run-server for more details"
Copy link
Member

Choose a reason for hiding this comment

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

I think we can skip the URL in the message. It will be very easy to forget about this later, making this a 404 if we update the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @anderseknert, will remove the url and fix tests.

christian1607
christian1607 reviewed Mar 11, 2022
View reviewed changes
server/server.go Outdated
@@ -1415,6 +1415,10 @@ func (s *Server) v1DataGet(w http.ResponseWriter, r *http.Request) {
DecisionID: decisionID,
}

if input == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this validates if the 'input' http query parameter is present in the request, the validation should be after JSON unmarshall is completed and validate if it has an 'input' field.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point, Christian! The tests should help shine some light on this :)

tsandall
tsandall requested changes Mar 18, 2022
View reviewed changes
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

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

Warning if the POST v1/data endpoint is called w/o providing input makes sense, but I'd rather not see that warning show up in GET v1/data.

@anderseknert
Copy link
Member

Agreed @tsandall 👍 @aflmp let me know if you want to address the issues mentioned here or if someone should pick up from where we are.

@aflmp
Copy link
Contributor Author

aflmp commented Mar 21, 2022

Agreed @tsandall 👍 @aflmp let me know if you want to address the issues mentioned here or if someone should pick up from where we are.

thank you @tsandall @anderseknert, pushed the changes based on the feedback.

aflmp
aflmp commented Mar 21, 2022
View reviewed changes
server/server_test.go
@@ -1463,6 +1533,16 @@ p = true { false }`
]
}`},
}},
{"post api usage warning", []tr{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added tests for warning but I can remove them if they look redundant

Copy link
Member

Choose a reason for hiding this comment

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

Nice! Thanks 👍

anderseknert
anderseknert approved these changes Mar 22, 2022
View reviewed changes
server/server_test.go
@@ -1463,6 +1533,16 @@ p = true { false }`
]
}`},
}},
{"post api usage warning", []tr{
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Thanks 👍

anderseknert
anderseknert previously approved these changes Mar 22, 2022
View reviewed changes
@anderseknert
Copy link
Member

LGTM, I'll let @tsandall merge this since there's an active change request from him, although AFAICS that's been addressed. Thanks for contributing @aflmp !

tsandall
tsandall previously approved these changes Mar 24, 2022
View reviewed changes
server: add warning when input attribute is missing
64c1262 
This change adds an api_usage_warning when input attribute is missing in the request to /v1/data

Fixes: open-policy-agent#4386
Signed-off-by: Alam <afaaqalam@gmail.com>

server: remove url from warning msg

Signed-off-by: Alam <afaaqalam@gmail.com>

server: remove warning for GET requests to v1/data; fix tests

Signed-off-by: Alam <afaaqalam@gmail.com>
@aflmp
Copy link
Contributor Author

aflmp commented Mar 24, 2022

rebased and squashed all the commits, ready to be merged.
thanks everyone

@anderseknert anderseknert merged commit 19450f8 into open-policy-agent:main Mar 24, 2022
@anderseknert
Copy link
Member

Thanks for your contribution, Alam! This will be helpful to many 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christian1607 christian1607 christian1607 left review comments

@tsandall tsandall tsandall left review comments

@anderseknert anderseknert anderseknert left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warn when input attributes is missing in /v1/data body
5 participants
@aflmp @anderseknert @tsandall @christian1607 @srenatus