Skip to content

Conversation

@adriendupuis
Copy link
Contributor

@adriendupuis adriendupuis commented Dec 28, 2022

Question Answer
JIRA Ticket
Versions 4.3 (ibexa/rest 4.3), 4.2 (ibexa/rest 4.2),
3.3 (ezsystems/ezplatform-rest 1.3), 2.5 (ezsystems/ezpublish-kernel 7.5)
  • Merge /user/groups/{id}/users into /user/groups/{path}/users: GET and POST of the same route were separated, both respective controller actions (loadUsersFromGroup and createUser) treat the argument as a path, and extract an ID from it.
  • Merge /user/groups/{id}/subgroups into /user/groups/{path}/subgroups: GET and POST of the same route, loadSubUserGroups and createUserGroup treat the argument as a path.

TODO

  • Generate HTML just before merge (after would have been easier for cherry picking)
  • Rebase to hide ez_original.raml edits

Checklist

  • Text renders correctly
  • Text has been checked with vale
  • Description metadata is up to date
  • Redirects cover removed/moved pages
  • Code samples are working
  • PHP code samples have been fixed with PHP CS fixer
  • Added link to this PR in relevant JIRA ticket or code PR

@adriendupuis adriendupuis changed the title Fix few REST API doc anomalies REST API ref: Merge /user/groups/{id}/ into /user/groups/{path}/ Dec 28, 2022
Copy link
Contributor Author

@adriendupuis adriendupuis Dec 29, 2022

Choose a reason for hiding this comment

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

The controller is extracting the ID from the path argument by splitting this path on slashes and keeping the last item ( extractLocationIdFromPath). So, it works with just the ID.

The description is not wrong but doesn't cover the whole possibilities. If having only a path, a developer shouldn't extract the ID from this path before use but directly use this path. Anyway, just having access to the path seems like a rare case to me, and an ID simpler to use.

This is a more general problem than just below /user/groups/{path}/;
Other routes having a {path} argument are in fact extracting the ID from it with their documentation not saying it.
From my PoV, the worst could be /content/locations/{path} description only documenting the path while its controller will extract the ID from this path. The 3 following example are equivalent but only the complex one is proposed:

  • /content/locations/1/2/61 (full path)
  • /content/locations/2/61 (incomplete path)
  • /content/locations/61 (ID)

I propose to not edit this description in this PR.

@adriendupuis adriendupuis marked this pull request as ready for review December 29, 2022 10:07
@adriendupuis adriendupuis marked this pull request as draft January 4, 2023 16:16
@adriendupuis adriendupuis marked this pull request as ready for review January 4, 2023 16:17
@Steveb-p
Copy link
Contributor

Steveb-p commented Jan 9, 2023

We shouldn't really encourage using "path" as the last argument, even if it works this way. Instead we should always declare that we expect an ID there.

@adriendupuis
Copy link
Contributor Author

adriendupuis commented Jan 9, 2023

We shouldn't really encourage using "path" as the last argument, even if it works this way. Instead we should always declare that we expect an ID there.

@Steveb-p I'm agree. So we could merge route entries the reverse way, both under /user/groups/{id}/ instead.
Edit: A bit complicated has there is many more descriptions to check. I'll try to do it in #1642

@adriendupuis adriendupuis merged commit d211e4a into master Jan 9, 2023
@adriendupuis adriendupuis deleted the fix-rest-api branch January 9, 2023 14:54
adriendupuis added a commit that referenced this pull request Feb 17, 2023
* ez-user-groups.raml: Merge /{id}/users into /{path}/users
* ez-user-groups.raml: Merge /{id}/subgroups into /{path}/subgroups

(cherry picked from commit d211e4a)
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.

2 participants