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

Provide custom namespace for creating things via POST route #1395

Merged
merged 3 commits into from
Jul 4, 2022
Merged

Provide custom namespace for creating things via POST route #1395

merged 3 commits into from
Jul 4, 2022

Conversation

Pranshu-G
Copy link
Contributor

@Pranshu-G Pranshu-G commented Jun 4, 2022 •
edited by thjaeckle
Loading

Signed-off-by: Pranshu-G pranshu.grover18@gmail.com

Fixes: #550 add custom namespace route.
Missing test case and documentation.

thjaeckle
thjaeckle requested changes Jun 13, 2022
View reviewed changes
Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

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

Hi @Pranshu-G and thx for your first contribution 👍
I added some comments to the added code.

Tests are still missing - at least the ThingIdTest should get a test for providing an explicit namespace.

Missing is also enhancing the OpenAPI documentation for POST /things here by adding a new parameter:
https://github.com/eclipse/ditto/blob/44a10bea1205eea177e8e48cbb69d0026b10509a/documentation/src/main/resources/openapi/sources/paths/things/index.yml#L80-L84

The complete OpenAPI can be built like documented here:
https://github.com/eclipse/ditto/tree/master/documentation/src/main/resources/openapi

Looking forward to the adjustments.

...ice/src/main/java/org/eclipse/ditto/gateway/service/endpoints/routes/things/ThingsRoute.java Outdated Show resolved Hide resolved
things/model/src/main/java/org/eclipse/ditto/things/model/ThingBuilder.java Outdated Show resolved Hide resolved
things/model/src/main/java/org/eclipse/ditto/things/model/ThingId.java Outdated Show resolved Hide resolved
.run/Ditto.run.xml Outdated Show resolved Hide resolved
...ice/src/main/java/org/eclipse/ditto/gateway/service/endpoints/routes/things/ThingsRoute.java Outdated Show resolved Hide resolved
@Pranshu-G
Copy link
Contributor Author

Pranshu-G commented Jun 15, 2022 •
edited
Loading

For the ThingIdTest, should I go about it by performing a POST request to the /things/namespace=my.cool.namespace with response as ThingId with namespace = my.cool.namespace followed by UUID ?

@thjaeckle
Copy link
Member

For the ThingIdTest, should I go about it by performing a POST request to the /things/namespace=my.cool.namespace with response as ThingId with namespace = my.cool.namespace followed by UUID ?

No, in the existing ThingIdTest you don't have the possibility to perform a POST request - you just unit test the newly added method in the class ThingId.

@thjaeckle thjaeckle added the on hold Waiting for feedback / requested changes on an issue/PR label Jun 27, 2022
@Pranshu-G
custom namespace /POST for things route
0cd1e2f 
Signed-off-by: Pranshu-G <pranshu.grover18@gmail.com>
@Pranshu-G
Copy link
Contributor Author

Pranshu-G commented Jun 30, 2022 •
edited
Loading

Rebased and Squashed
PTAL @thjaeckle

Edit: I suppose there is something wrong with the openAPI enhancement I made.

@Pranshu-G
add license year 2022
94500d4 
Signed-off-by: Pranshu-G <pranshu.grover18@gmail.com>
Pranshu-G
Pranshu-G commented Jul 1, 2022
View reviewed changes
Copy link
Contributor Author

@Pranshu-G Pranshu-G left a comment

Choose a reason for hiding this comment

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

I added a reference to namespace parameter to ditto-api-2.yml but I am not sure it is reflecting the desired behavior

@Pranshu-G Pranshu-G marked this pull request as ready for review July 1, 2022 10:38
@Pranshu-G Pranshu-G requested a review from thjaeckle July 1, 2022 13:24
@thjaeckle thjaeckle removed the on hold Waiting for feedback / requested changes on an issue/PR label Jul 4, 2022
@thjaeckle
review:
1fc3ec4 
* did some code formatting
* fixed OpenAPI documentation by adding "Namespace" parameter
* adjusted some working in OpenAPI docs as well
* fixed non-related typo in wot-thingDescription.yml

Signed-off-by: Thomas Jaeckle <thomas.jaeckle@bosch.io>
thjaeckle
thjaeckle approved these changes Jul 4, 2022
View reviewed changes
Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

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

Hi @Pranshu-G
Thanks for the adjustments - I had another look and added a "review commit".
Could you check if that is ok for you?
I just applied some formatting and fixed the OpenAPI docs by adding the "Namespace" parameter separately.

@thjaeckle thjaeckle added this to the 3.0.0 milestone Jul 4, 2022
@Pranshu-G
Copy link
Contributor Author

Hi @Pranshu-G Thanks for the adjustments - I had another look and added a "review commit". Could you check if that is ok for you? I just applied some formatting and fixed the OpenAPI docs by adding the "Namespace" parameter separately.

Hi @thjaeckle , the changes look fine. The OpenAPI change was necessary since I wasn't able to get the desired output there. Is the PR good to go now?

@thjaeckle
Copy link
Member

Is the PR good to go now?

Yes, once the build is "green" we can accept and merge it 👍

@thjaeckle thjaeckle merged commit f4faa6e into eclipse-ditto:master Jul 4, 2022
@thjaeckle thjaeckle changed the title [WIP]custom namespace /POST for things route Provide custom namespace for creating things via POST route Jul 4, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thjaeckle thjaeckle thjaeckle approved these changes

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

Successfully merging this pull request may close these issues.

Add POST things with specified namespace to HTTP API
2 participants
@Pranshu-G @thjaeckle