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

MF-1357 - Add new endpoint for searching things #1383

Merged
merged 13 commits into from
Mar 11, 2021
Merged

Conversation

blokovi
Copy link
Contributor

@blokovi blokovi commented Mar 5, 2021

Signed-off-by: Ivan Milosevic iva@blokovi.com

What does this do?

Add /things/search POST endpoint with filter parameters for name and metadata in json body

Which issue(s) does this PR fix/relate to?

Resolves #1357

List any changes that modify/break current functionality

Have you included tests for your changes?

Yes

Did you document any new/modified functionality?

Notes

@blokovi blokovi requested a review from a team as a code owner March 5, 2021 08:08
@@ -105,6 +105,13 @@ func MakeHandler(tracer opentracing.Tracer, svc things.Service) http.Handler {
opts...,
))

r.Post("/things/list", kithttp.NewServer(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a search here /things/search? Like search is a resource itself. Then POST makes sense because you are creating a new search resource with a req body (containing search criteria).

@@ -180,6 +180,13 @@ type listResourcesReq struct {
pageMetadata things.PageMetadata
}

type listResourcesMetaReq struct {
token string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which token is this?
User token is in req header, thing doesn't have a token, it has key (in domain language we don't call it token).

'404':
description: A non-existent entity request.
'422':
description: Database can't process request.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer just Unprocessable Entity here a "standard" 422 status message, if you can't say more (like which field etc...). Without mentioning Database...

things/service_test.go Outdated Show resolved Hide resolved
@@ -72,7 +72,7 @@ func TestCreateThings(t *testing.T) {
key: token,
err: nil,
event: map[string]interface{}{
"id": "1",
"id": "001",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to use real IDs

Comment on lines 600 to 632
ThingsReqSchema:
type: object
properties:
name:
type: string
description: Free-form thing name.
metadata:
type: object
description: Object-encoded thing's data.
total:
type: integer
description: Total number of items.
offset:
type: integer
description: Number of items to skip during retrieval.
limit:
type: integer
description: Maximum number of items to return in one page.
order:
type: string
description: Order type.
dir:
type: string
description: Order direction.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be described better.
Follow this existent example and adapt it to use params from the body:

    Limit:
      name: limit
      description: Size of the subset to retrieve.
      in: query
      schema:
        type: integer
        default: 10
        maximum: 100
        minimum: 1
      required: false
    Offset:
      name: offset
      description: Number of items to skip during retrieval.
      in: query
      schema:
        type: integer
        default: 0
        minimum: 0
      required: false
    Connected:
      name: connected
      description: Connection state of the subset to retrieve.
      in: query
      schema:
        type: boolean
        default: true
      required: false
    Name:
      name: name
      description: Name filter. Filtering is performed as a case-insensitive partial match.
      in: query
      schema:
        type: string
      required: false
    Order:
      name: order
      description: Order type.
      in: query
      schema:
        type: string
        default: id
        enum:
          - name
          - id
      required: false
    Direction:
      name: dir
      description: Order direction.
      in: query
      schema:
        type: string
        default: desc
        enum:
          - asc
          - desc
      required: false
    Metadata:
      name: metadata
      description: Metadata filter. Filtering is performed matching the parameter with metadata on top level. Parameter is json.
      in: query
      required: false
      schema:
        type: object
        additionalProperties: {}

things/mocks/things.go Show resolved Hide resolved
things/mocks/channels.go Show resolved Hide resolved
things/api/things/http/transport.go Outdated Show resolved Hide resolved
@@ -36,7 +35,7 @@ const (
var (
metadata = map[string]interface{}{"meta": "data"}
metadata2 = map[string]interface{}{"meta": "data2"}
thing = sdk.Thing{ID: "1", Name: "test_device", Metadata: metadata}
thing = sdk.Thing{ID: "001", Name: "test_device", Metadata: metadata}
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to use UUID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point is that here we mock id so we can test "order by id" functionalities.
That is the reason why I have change mock IDs to use three chars instead one, because ordering with just one char, like it is now, is 1, 10, 100, 2, 20...
With this change order would be 001, 002, 003... and you can write tests that check order by id functionality.

@@ -16,7 +15,7 @@ import (
)

var (
channel = sdk.Channel{ID: "1", Name: "test"}
channel = sdk.Channel{ID: "001", Name: "test"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use UUID?

nmarcetic
nmarcetic previously approved these changes Mar 9, 2021
nmarcetic
nmarcetic previously approved these changes Mar 10, 2021
@@ -49,6 +49,10 @@ var (
invalidName = strings.Repeat("m", maxNameSize+1)
notFoundRes = toJSON(errorRes{things.ErrNotFound.Error()})
unauthRes = toJSON(errorRes{things.ErrUnauthorizedAccess.Error()})
searchThReq = things.PageMetadata{
Copy link
Contributor

Choose a reason for hiding this comment

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

searchThingReq

Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Use same request as list endpoint

Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Add swagger file

Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Signed-off-by: Ivan Milosevic <iva@blokovi.com>
fix tests

Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Signed-off-by: Ivan Milosevic <iva@blokovi.com>
fix tracer string
change test offset

Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Signed-off-by: Ivan Milosevic <iva@blokovi.com>
Copy link
Contributor

@manuio manuio left a comment

Choose a reason for hiding this comment

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

LGTM

@manuio manuio merged commit 30ba38c into absmach:master Mar 11, 2021
@blokovi blokovi deleted the MF-1357 branch March 11, 2021 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata in URL query not being properly parsed
4 participants