Area API for creating hierarchical area/sub-area in a space. #655

sbose78 · 2017-01-13T06:58:51Z

An Area is a hierarchical structure that belongs to a Space.

As part of this PR, I added REST capabilities to

create a new Area in a space. [ User Story ] Add API to create Area in a specific Space #688
create a child-Area in a space. [ User story ] Add API to Create sub/child Area in a space #689
list areas by space. [ User story ] Add API to List Areas by Space #690

Design decisions:

The datastype 'ltree' is used to store the hierarchy in the postgresql database in the Areas table.
The hierarchy is stored as dot-delimited list of areas for child areas.
The UUID is stored in the hierarchy of areas in the "Path" column in Areas table.
The UUID is a set of alphanumeric characters in the format xxxyyxx-yyyy-uuu3u-hjkh434 however the ltree type in postgres supports only the "" special character and not the "-" character. Hence UUIDs are converted into the 'ltree' format by converting "-" to "" and vice versa when needed.

Fixes #688 , #689 and #690

codecov-io · 2017-01-13T07:30:58Z

Codecov Report

Merging #655 into master will decrease coverage by -0.02%.
The diff coverage is 71.69%.

@@            Coverage Diff            @@
##           master    #655      +/-   ##
=========================================
- Coverage   72.12%   72.1%   -0.02%     
=========================================
  Files          80      83       +3     
  Lines        4760    4972     +212     
=========================================
+ Hits         3433    3585     +152     
- Misses        996    1041      +45     
- Partials      331     346      +15

Impacted Files	Coverage Δ
main.go	`0% <ø> (ø)`	✅
migration/migration.go	`52.5% <100%> (+0.19%)`	✅
space-areas.go	`44% <44%> (ø)`
area.go	`76.8% <76.8%> (ø)`
area/area.go	`77.19% <77.19%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fad105...ce10325. Read the comment docs.

kwk · 2017-01-13T19:37:52Z

[test]

sbose78 · 2017-01-23T22:00:07Z

Scenario: Create a space and an area under it

1. Create a Space under which the areas and sub areas would be created.


$ /bin/alm-cli create space --key SOME_TOKEN  --payload '{ "data":{ "type":"spaces" ,  "attributes":{"name":"redhat-engg"}}}' -H localhost:8080 --pp

2017/01/24 03:01:02 [INFO] started id=Z96NlmWw POST=http://localhost:8080/api/spaces
2017/01/24 03:01:02 [INFO] completed id=Z96NlmWw status=201 time=6.862782ms
{
    "data": {
        "attributes": {
            "created-at": "2017-01-24T03:01:02.699539+05:30",
            "name": "redhat-engg",
            "updated-at": "2017-01-24T03:01:02.699539+05:30",
            "version": 0
        },
        "id": "41a88c1c-e2ec-48f4-b25f-d86e52eb06ea",
        "links": {
            "self": "http://localhost:8080/api/spaces/41a88c1c-e2ec-48f4-b25f-d86e52eb06ea"
        },
        "relationships": {
            "iterations": {
                "links": {
                    "related": "http://localhost:8080/api/spaces/41a88c1c-e2ec-48f4-b25f-d86e52eb06ea/iterations"
                }
            }
        },
        "type": "spaces"
    }

2. Create an area under the space created above

$ ./bin/alm-cli create space-areas --key $ALM_TOKEN --id  "f214c5d7-1166-4a05-9b43-9e82394d818c"  --payload '{ "data":{"type":"areas","attributes":{"name":"area1"} }}' -H localhost:8080 --pp

2017/02/09 00:03:18 [INFO] started id=tNzPEmy9 POST=http://localhost:8080/api/spaces/f214c5d7-1166-4a05-9b43-9e82394d818c/Areas
2017/02/09 00:03:18 [INFO] completed id=tNzPEmy9 status=201 time=30.698729ms
{
    "data": {
        "attributes": {
            "created-at": "2017-02-09T00:03:18.168278+05:30",
            "name": "area1",
            "parent_path": "/",
            "parent_path_resolved": "/",
            "version": 0
        },
        "id": "2a1ebc01-4a0a-4fd8-a79f-92ed3e562d46",
        "links": {
            "self": "http://localhost:8080/api/areas/2a1ebc01-4a0a-4fd8-a79f-92ed3e562d46"
        },
        "relationships": {
            "children": {
                "links": {
                    "self": "http://localhost:8080/api/areas/2a1ebc01-4a0a-4fd8-a79f-92ed3e562d46/children"
                }
            },
            "space": {
                "data": {
                    "id": "f214c5d7-1166-4a05-9b43-9e82394d818c",
                    "type": "spaces"
                },
                "links": {
                    "self": "http://localhost:8080/api/spaces/f214c5d7-1166-4a05-9b43-9e82394d818c"
                }
            }
        },
        "type": "areas"
    }
}

Scenario : Create area under an area, basically a sub-area.

./bin/alm-cli create space --key $ALM_TOKEN  --payload '{ "data":{ "type":"spaces" ,  "attributes":{"name":"redhat-engg2"}}}' -H localhost:8080 --pp

The json-api response looks like this.

2017/02/08 23:37:07 [INFO] started id=qAGfoiby POST=http://localhost:8080/api/spaces
2017/02/08 23:37:07 [INFO] completed id=qAGfoiby status=201 time=8.591655ms
{
    "data": {
        "attributes": {
            "created-at": "2017-02-08T23:37:07.113433+05:30",
            "description": "",
            "name": "redhat-engg2",
            "updated-at": "2017-02-08T23:37:07.113433+05:30",
            "version": 0
        },
        "id": "f214c5d7-1166-4a05-9b43-9e82394d818c",
        "links": {
            "self": "http://localhost:8080/api/spaces/f214c5d7-1166-4a05-9b43-9e82394d818c"
        },
        "relationships": {
            "iterations": {
                "links": {
                    "related": "http://localhost:8080/api/spaces/f214c5d7-1166-4a05-9b43-9e82394d818c/iterations"
                }
            }
        },
        "type": "spaces"
    }
}

In the database column "path",
the value is stored as

528bda88_f458_40bf_8e76_925cbca5960c.ceb17d71_9eea_4b4d_9c51_65814e4dfc28.ccb325e4_417c_4370_8181_f47ccc0a15db

The hierarchy is dot delimited, the uuids of the parent area(s) is stored by replacing the "-" with "_" because the "-" character is not supported in the ltree field.

** Scenario : List all children of a specific area **

sbose78 · 2017-01-24T17:00:29Z

[test]

kwk · 2017-01-25T08:52:26Z

[test]

kwk · 2017-01-25T10:29:24Z

[test]

aslakknutsen · 2017-01-28T10:18:43Z

area.go

+	spaceID := area.SpaceID.String()
+
+	selfURL := rest.AbsoluteURL(request, app.AreaHref(area.ID))
+	spaceSelfURL := rest.AbsoluteURL(request, "/api/spaces/"+spaceID)


SpaceHref ?

Thanks for pointing out - done here, 39a653f

aslakknutsen · 2017-01-28T10:29:06Z

migration/migration.go

@@ -139,6 +139,9 @@ func getMigrations() migrations {
 	// Version 20
 	m = append(m, steps{executeSQLFile("020-work-item-description-update-search-index.sql")})

+	// Version 21
+	m = append(m, steps{executeSQLFile("021-areas.sql")})


Update to 22?

aslakknutsen · 2017-01-28T10:30:00Z

space-areas_test.go

+	return svc, NewSpaceAreasController(svc, rest.db)
+}
+
+func (rest *TestSpaceAreaREST) TestSuccessCreateArea() {


Add some non happy case tests, e.g. 404 on create area in missing space. Missing auth on create..

Sure @aslakknutsen - was waiting to have my approach reviewed before I did that. Thanks for the review.

Added negative tests: 1104884

aslakknutsen · 2017-01-28T10:34:27Z

design/areas.go

+
+var areaAttributes = a.Type("AreaAttributes", func() {
+	a.Description(`JSONAPI store for all the "attributes" of a Area. +See also see http://jsonapi.org/format/#document-resource-object-attributes`)
+	a.Attribute("name", d.String, "The Area name", func() {


Expose created-at

Expose path, but resolved by Name.

name: Child2
path: Area1.Child2.Child2

Maybe both 'path' as uuid and 'path' as resolved.

Exposed CreatedAt and Version ( e48559a )

@aslakknutsen , does this mean that in the request parameter, only path would be passed as area1.child1.child2
and internally we would be persisting in the database both the name( derived ) and path ?

Maybe both 'path' as uuid and 'path' as resolved.

Didn't understand this - Could you please explain ?

As discussed, the following needs to be exposed, agnostic to how we persist in the database:

path ( of UUIDs upto topmost parent )

path_resolved ( of area_names of the above UUIDs )

relationship : link to all children

relationship : link to parent.

aslakknutsen · 2017-01-31T07:47:45Z

migration/sql-files/021-areas.sql

@@ -0,0 +1,13 @@
+CREATE EXTENSION IF NOT EXISTS "ltree";


Why 2 migration scripts?

Silly me, removed.

pranavgore09 · 2017-02-01T09:43:47Z

area/area.go

+	defer goa.MeasureSince([]string{"goa", "db", "area", "create"}, time.Now())
+
+	u.ID = uuid.NewV4()
+


Should default value for Path be ConvertToLltreeFormat(u.ID.String()) ?

if u.Path == ""{ u.Path = ConvertToLltreeFormat(u.ID.String()) }

Ref:- default path for base WIT is itself.

postgres=# select name,path from work_item_types where name = 'planneritem'; name | path -------------+------------- planneritem | planneritem (1 row)

Thanks for the feedback @pranavgore09 - yes we could, but the path is the parent path - the name has been renamed at the service level to parent_path..

alexeykazakov · 2017-02-01T03:30:17Z

design/areas.go

+})
+
+// new version of "list" for migration
+var _ = a.Resource("space-areas", func() {


a.Resource("space-area", func() { a.BasePath("/space-areas")

for the sake of consistency?

Thanks for your feedback @alexeykazakov ,

Sorry if I'm missing something silly and obvious !
I don't quite understand what is the advantage of doing so. I checked `design/iterations.go' and we do not add the base path mentioned by you.

./bin/alm-cli create space-areas --key $ALM_TOKEN --id "f214c5d7-1166-4a05-9b43-9e82394d818c" --payload '{ "data":{"type":"areas","attributes":{"name":"area1"} }}' -H localhost:8080 --pp 2017/02/08 23:37:59 [INFO] started id=yND1mdkC POST=http://localhost:8080/api/spaces/f214c5d7-1166-4a05-9b43-9e82394d818c/Areas 2017/02/08 23:37:59 [INFO] completed id=yND1mdkC status=201 time=8.446925ms { "data": { "attributes": { "created-at": "2017-02-08T23:37:59.644013+05:30", "name": "area1", "parent_path": "/", "version": 0 }, "id": "256db834-a620-46b8-9322-808ee983c8a7", "links": { "self": "http://localhost:8080/api/areas/256db834-a620-46b8-9322-808ee983c8a7" }, "relationships": { "children": { "links": { "self": "http://localhost:8080/api/areas/256db834-a620-46b8-9322-808ee983c8a7/children" } }, "space": { "data": { "id": "f214c5d7-1166-4a05-9b43-9e82394d818c", "type": "spaces" }, "links": { "self": "http://localhost:8080/api/spaces/f214c5d7-1166-4a05-9b43-9e82394d818c" } } }, "type": "areas" } }

The above is how the request looks like , how do you suggest we design the url paths in the API?

./bin/alm-cli create space-area instead of ./bin/alm-cli create space-areas
as we do for space: ./bin/alm-cli create space
As I said, just for the sake of consistency ;)

Thanks @alexeykazakov , I was following space-iterations for consistency - If u suggest I could update stuff of space-area here and space-iteration in another PR :)

OK. so we have more inconsistency than I though ;)

…-services#655) Fixes fabric8-services#688 Fixes fabric8-services#689 Fixes fabric8-services#690

sbose78 force-pushed the area-service branch from c45fb3e to 20dcd22 Compare January 13, 2017 07:36

sbose78 force-pushed the area-service branch 2 times, most recently from 0194224 to aa19829 Compare January 23, 2017 18:52

sbose78 force-pushed the area-service branch from 251aca3 to 60741a3 Compare January 24, 2017 08:52

sbose78 changed the title ~~WIP : Area service~~ WIP : Area API for creating hierarchical area/sub-area in a space. Jan 24, 2017

sbose78 force-pushed the area-service branch 3 times, most recently from 5e5b403 to e93121b Compare January 24, 2017 16:32

sbose78 changed the title ~~WIP : Area API for creating hierarchical area/sub-area in a space.~~ Area API for creating hierarchical area/sub-area in a space. Jan 25, 2017

sbose78 requested a review from kwk January 25, 2017 07:24

sbose78 requested a review from aslakknutsen January 25, 2017 09:05

kwk mentioned this pull request Jan 25, 2017

Increase build timeout of coverage builds by 5 min openshiftio/openshiftio-cico-jobs#64

Merged

sbose78 force-pushed the area-service branch from 85e1911 to 56305c8 Compare January 25, 2017 15:32

sbose78 requested review from xcoulon and alexeykazakov January 26, 2017 10:00

aslakknutsen suggested changes Jan 28, 2017

View reviewed changes

sbose78 force-pushed the area-service branch from 56305c8 to 163c85b Compare January 30, 2017 07:01

aslakknutsen suggested changes Jan 31, 2017

View reviewed changes

sbose78 force-pushed the area-service branch 2 times, most recently from c300f31 to e8c91bb Compare February 1, 2017 02:49

pranavgore09 reviewed Feb 1, 2017

View reviewed changes

alexeykazakov suggested changes Feb 3, 2017

View reviewed changes

sbose78 added 26 commits February 13, 2017 00:26

added negative tests

28ee106

remove old migration script

743cd39

change variable name so as to not conflict with package name

129ae5c

remove space import

cf36d4f

add design changes for path and path resolved attr , children api

1edce71

Add service for showing child areas

04cdd6d

add dao for getting multiple areas

d92d1cc

add link for child

a9af160

rename migration file

3f101aa

handle changes in gormsupport cleaner and test account identity

b4fbdc1

fix changes in test identity

2b9b9a0

fix another identity issue

8292016

list children uses the upward tree for search

d9efa2d

refactor to addResolvePath

c44d3b0

use / as path separator

5086d0f

fix context import for the nth time :(

6088187

change path --> parent_path

321011f

add path resolution to space areas

94ee3e1

cleanup, fix example in design

f6bf456

add validation for area details

3e8dc27

lowercase url path resource

54c14e4

move out duplicate methods

9e218e0

moved areas migration down the list

912114b

fix merge issues

875f586

remove use of pub key in tests

f887141

pranav feedback on ordering of area

ce10325

sbose78 force-pushed the area-service branch from 8464e8d to ce10325 Compare February 12, 2017 18:56

aslakknutsen approved these changes Feb 14, 2017

View reviewed changes

aslakknutsen merged commit 26af244 into fabric8-services:master Feb 14, 2017

AdamJ pushed a commit to AdamJ/almighty-core that referenced this pull request Mar 7, 2017

Area API for creating hierarchical area/sub-area in a space. (fabric8…

1094909

…-services#655) Fixes fabric8-services#688 Fixes fabric8-services#689 Fixes fabric8-services#690

Area API for creating hierarchical area/sub-area in a space. #655

Area API for creating hierarchical area/sub-area in a space. #655

sbose78 commented Jan 13, 2017 •

edited

codecov-io commented Jan 13, 2017 •

edited

kwk commented Jan 13, 2017

sbose78 commented Jan 23, 2017 •

edited

sbose78 commented Jan 24, 2017

kwk commented Jan 25, 2017

kwk commented Jan 25, 2017

aslakknutsen Jan 28, 2017

sbose78 Jan 30, 2017

aslakknutsen Jan 28, 2017

aslakknutsen Jan 28, 2017

sbose78 Jan 30, 2017

sbose78 Jan 30, 2017

aslakknutsen Jan 28, 2017

aslakknutsen Jan 28, 2017

sbose78 Jan 30, 2017

sbose78 Jan 30, 2017 •

edited

sbose78 Feb 3, 2017

aslakknutsen Jan 31, 2017

sbose78 Jan 31, 2017

pranavgore09 Feb 1, 2017 •

edited

sbose78 Feb 8, 2017

alexeykazakov Feb 1, 2017

sbose78 Feb 8, 2017 •

edited

alexeykazakov Feb 8, 2017

sbose78 Feb 10, 2017

alexeykazakov Feb 10, 2017

		defer goa.MeasureSince([]string{"goa", "db", "area", "create"}, time.Now())

		u.ID = uuid.NewV4()

Area API for creating hierarchical area/sub-area in a space. #655

Area API for creating hierarchical area/sub-area in a space. #655

Conversation

sbose78 commented Jan 13, 2017 • edited

codecov-io commented Jan 13, 2017 • edited

Codecov Report

kwk commented Jan 13, 2017

sbose78 commented Jan 23, 2017 • edited

sbose78 commented Jan 24, 2017

kwk commented Jan 25, 2017

kwk commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 Jan 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavgore09 Feb 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 Feb 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 commented Jan 13, 2017 •

edited

codecov-io commented Jan 13, 2017 •

edited

sbose78 commented Jan 23, 2017 •

edited

sbose78 Jan 30, 2017 •

edited

pranavgore09 Feb 1, 2017 •

edited

sbose78 Feb 8, 2017 •

edited