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.

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

New API: list vulnerabilities by namespace #82

Merged
merged 4 commits into from
Mar 8, 2016
Merged

New API: list vulnerabilities by namespace #82

merged 4 commits into from
Mar 8, 2016

Conversation

liangchenye
Copy link
Contributor

#80

Signed-off-by: liangchenye liangchenye@huawei.com

@@ -141,6 +141,9 @@ const (
searchVulnerabilityForUpdate = ` FOR UPDATE OF v`
searchVulnerabilityByNamespaceAndName = ` WHERE n.name = $1 AND v.name = $2 AND v.deleted_at IS NULL`
searchVulnerabilityByID = ` WHERE v.id = $1`
searchVulnerabilityByNamespace = ` WHERE n.name = $1 AND v.deleted_at IS NULL
ORDER BY v.name
LIMIT $2 offset $3`
Copy link
Contributor

Choose a reason for hiding this comment

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

To ensure good performances at scale, we try to avoid using OFFSET but instead use WHERE id >= $3 ORDER BY l.ID LIMIT $2. And then to avoid leaking that internal identifier, we encrypt the id at the API level. See how notifications work.

@Quentin-M
Copy link
Contributor

Thanks! I just wrote some comments ~

@liangchenye
Copy link
Contributor Author

Update by encrypting page, just like notificationPage.
I added numberToToken/tokenToNumber, and NextPage *string pointer to VulnerabilityEnvelope .

@@ -316,3 +319,24 @@ func pageNumberToToken(page database.VulnerabilityNotificationPageNumber, key st

return string(tokenBytes)
}

func tokenToNumber(token, key string) (int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could definitely only have one function for marshaling and one function for unmarshaling for tokens. Just make it only return an error and you can pass a newly allocated type by reference similar to normal JSON unmarshalling.

Signed-off-by: liangchenye <liangchenye@huawei.com>
Signed-off-by: liangchenye <liangchenye@huawei.com>
Signed-off-by: liangchenye <liangchenye@huawei.com>
@liangchenye
Copy link
Contributor Author

@jzelinskie good idea! updated by using tokenMarshal/tokenUnmarshal

@@ -215,7 +215,8 @@ func NotificationFromDatabaseModel(dbNotification database.VulnerabilityNotifica

var nextPageStr string
if nextPage != database.NoVulnerabilityNotificationPage {
nextPageStr = pageNumberToToken(nextPage, key)
nextPageBytes, _ := tokenMarshal(nextPage, key)
Copy link
Contributor

Choose a reason for hiding this comment

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

gofmt -s would rewrite this to nextPageBytes := pageNumberToToken(nextPage, key)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't get it.

Copy link
Contributor

Choose a reason for hiding this comment

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

run gofmt -s on this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but nothing changes.
And pageNumberToToken is already replaced by tokenMarshal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! I'm sorry for the confusion. Usually gofmt -s drops the ignored value if it's second.

…d error

Signed-off-by: liangchenye <liangchenye@huawei.com>
@@ -8,6 +8,7 @@
- [Namespaces](#namespaces)
- [GET](#get-namespaces)
- [Vulnerabilities](#vulnerabilities)
- [List](#get-namespacesnsnamevulnerabilities)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish there was a better way to put this, because I was using HTTP verbs for all of these and technically this is a GET

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but how though?

@Quentin-M
Copy link
Contributor

I think we're pretty much ready to merge that PR @jzelinskie? Except if you can find some better wording for the API docs.

Again, thanks a lot @liangchenye for this great and much appreciated contribution! That's awesome, we're all very excited 👍

@jzelinskie
Copy link
Contributor

LGTM

Quentin-M added a commit that referenced this pull request Mar 8, 2016
*: list vulnerabilities by namespace
@Quentin-M Quentin-M merged commit 7bea9cb into quay:master Mar 8, 2016
@jzelinskie jzelinskie added kind/feature request wishes for new functionality/docs reviewed/lgtm labels Mar 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature request wishes for new functionality/docs
Development

Successfully merging this pull request may close these issues.

None yet

3 participants