Problem/motivation

When JSON:API is set to readonly, the results of the openapi module still defines the endpoints for creating, updating and deleting resources.

This makes the documentation a bit weird since the endpoints don't really work but are documented.

Proposed solution

It seems the routes from the`getJsonApiRoutes` method that are used in the `JsonApiGenerator` returns routes with all methods enabled even though only `GET` is allowed when switching the `jsonapi` to readonly. This should not be the case.

It should probably be fixed in `JsonApiGenerator` to check if readonly is enabled in the config.

CommentFileSizeAuthor
#29 hide-post-put-delete-3079209-29.patch1.32 MBbharath-kondeti
#27 hide-post-put-delete-3079209-27.patch1.22 KBbroon
#21 hide-post-put-delete-3079209-21.patch620.7 KBhitchshock
#12 only_add_read_route-methods_when_read_only_is_enabled_in_jsonapi_8.x_2.x-3079209-11.patch1.23 KBabu-zakham
#11 only_add_read_route-methods_when_read_only_is_enabled_in_jsonapi_3079209-10.patch1.22 KBabu-zakham
#10 add_required_keyword_8.x_2.x-3185778-5.patch927 bytesabu-zakham
#5 3079209-5-reroll.patch1.23 KBrichgerdes
#3 only_add_read_route-methods_when_read_only_is_enabled_in_jsonapi_3079209-3.patch1.29 KBbbrala
#2 Only_add_read_route-methods_when_read_only_is_enabled_in_jsonapi-3079209-1.patch1.28 KBbbrala

Issue fork openapi_jsonapi-3079209

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

  • 3079209-hide-post-put Comparechanges, plain diff MR !6
    • 2 hidden branches
  • 3.x Comparecompare
  • 8.x-2.x Comparecompare
About issue forks

Comments

bbrala created an issue. See original summary.

bbrala’s picture

bbrala
Location Netherlands
commented
StatusFileSize
new Only_add_read_route-methods_when_read_only_is_enabled_in_jsonapi-3079209-1.patch1.28 KB

I've made a possible fix, although no tests yet. Would this be the proper place to fix this?

bbrala’s picture

bbrala
Location Netherlands
commented
StatusFileSize
new only_add_read_route-methods_when_read_only_is_enabled_in_jsonapi_3079209-3.patch1.29 KB

I've fixed some glaring codestyle issues for the patch in comment #2

bbrala’s picture

bbrala
Location Netherlands
commented
richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented
Status: Active » Needs review
StatusFileSize
new 3079209-5-reroll.patch1.23 KB

I had to fix the diff header to get it to apply correctly. Attached is the rerolled patch.

Lets run tests to verify this doesn't break anything

Status: Needs review » Needs work

The last submitted patch, 5: 3079209-5-reroll.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
commented

The patch does work, but I hadn't considered how the tests would react. Since jsonapi defaults to read only mode, I will need to write tests, or at least alter the existing tests.. They should be simple, I'll try to get to this later today. Basically, the only change to the existing test is changing the jsonapi config to non read only mode. We should then add a secondary test for jsonapi, which verifies that with the default config (read only mode) the only method is get.

bbrala’s picture

bbrala
Location Netherlands
commented

Thanks for taking the time.

If I knew how to run tests locally for modules I'd do that but I don't.

Seems reasonable to split the tests to read only mode on and off at some point.

abu-zakham’s picture

abu-zakham commented
Project: OpenAPI » OpenAPI for JSON:API
Version: 8.x-1.x-dev » 3.0.1
abu-zakham’s picture

abu-zakham commented
StatusFileSize
new add_required_keyword_8.x_2.x-3185778-5.patch927 bytes
abu-zakham’s picture

abu-zakham commented
StatusFileSize
new only_add_read_route-methods_when_read_only_is_enabled_in_jsonapi_3079209-10.patch1.22 KB

Re-rolled patch for openapi_jsonapi

abu-zakham’s picture

abu-zakham commented
StatusFileSize
new only_add_read_route-methods_when_read_only_is_enabled_in_jsonapi_8.x_2.x-3079209-11.patch1.23 KB

Patch for 8.x-2.x branch

abu-zakham’s picture

abu-zakham commented
Status: Needs work » Needs review
guypaddock’s picture

guypaddock commented
Title: Only show enabled endpoints for JSON:API » Hide POST, PUT, and DELETE endpoints when JSON:API is configured to be read-only

Corrected title to be more precise, because endpoints disabled via JSON:API Extras definitely do not appear in the documentation. So, looks like this is specifically about hiding mutable resources from the docs.

rajab natshah’s picture

rajab natshah
he/him
commented
Status: Needs review » Reviewed & tested by the community
bbrala’s picture

bbrala
Location Netherlands
commented
Status: Reviewed & tested by the community » Needs review

Think that statuschange is not fair rajab, tests fail, i see no comments from you. Think this is not RTBC to be honest.

rajab natshah’s picture

rajab natshah
he/him
commented

Thank you, Björn for following up on the issue.
We had been working with patch #12 for around 8 months.
And I did a test for fresh Drupal sites.
#3189326: Add 2 patches for the OpenAPI for JSON:API module to fix issues with endpoints when JSON:API is configured to be read-only
The patch apply on the 3.0.x branch too

guypaddock’s picture

guypaddock commented
Status: Needs review » Needs work

@Rajab: automated tests, not manual tests. The patch doesn't update the automated tests, so they are failing with this patch. Back to NW.

HitchShock made their first commit to this issue’s fork.

HitchShock opened merge request !6

hitchshock’s picture

hitchshock
Location Ukraine
commented
StatusFileSize
new hide-post-put-delete-3079209-21.patch620.7 KB

Updated tests according to the last changes. Also created merge request.

hitchshock’s picture

hitchshock
Location Ukraine
commented
Status: Needs work » Needs review
bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Reviewed & tested by the community

Although the changes to uuid are a bit puzzling to me the changes do look good. Testing this way also seems like it's correct so I'm gonna give it a rtbc (even though I worked out this a while back :))

broon’s picture

broon
Location Potsdam
commented

I did need the same functionality and already hacked in JsonApiGenerator class to create a PoC. Before I decorated that service in a custom module, I stumbled upon this issue and patch and can confirm that it works as expected.

vacilando’s picture

vacilando commented

Could this please be merged to DEV?

psf_’s picture

psf_ commented
Status: Reviewed & tested by the community » Needs work

The last patch don't merge with openapi_jsonapi:3.0.4

I see too that the last patch add changes not related with this issue.

broon’s picture

broon
Location Potsdam
commented
Version: 3.0.1 » 3.0.4
Status: Needs work » Needs review
StatusFileSize
new hide-post-put-delete-3079209-27.patch1.22 KB

Re-roll patch #12 against 3.0.4

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

Thanks for the reroll, but you forgot to include the changes from #21

bharath-kondeti’s picture

bharath-kondeti commented
Status: Needs work » Needs review
StatusFileSize
new hide-post-put-delete-3079209-29.patch1.32 MB

Addressed #28 and updated the changes from #21. Please review

bbrala’s picture

bbrala
Location Netherlands
commented

When posting patches try and add and interdiff, it does sometimes fail when resolving though, then I wouldn't add it

https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

rajab natshah’s picture

rajab natshah
he/him
commented
Status: Needs review » Reviewed & tested by the community
bradjones1’s picture

bradjones1
Primary language English
commented
Version: 3.0.4 » 3.x-dev
Status: Reviewed & tested by the community » Needs work
ricovandevin’s picture

ricovandevin commented

Patch from #29 seems to work fine. Leaving status to Needs work until we have a green test result. Queued patch for retest.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Related issues: +#3173157: OpenAPI support

sokru made their first commit to this issue’s fork.

sokru changed the visibility of the branch 8.x-2.x to hidden.

sokru changed the visibility of the branch 3.x to hidden.

sokru’s picture

sokru commented
Status: Needs work » Needs review

Rebased the MR and fixed few coding standards related issues. A bit unsure about the test coverage, since the tests seem to pass even if I mix the contents of test/expectations/read_only_* directories.

phernand42’s picture

phernand42 commented

Patch from #29 also working for me too.

rajab natshah’s picture

rajab natshah
he/him
commented
Status: Needs review » Reviewed & tested by the community