Problem/Motivation

Json api by default uses the prefix /jsonapi. That was hard coded into the OpenAPI implementation of the JsonAPI specs. The JsonApi Extras module allows the user to change the api prefix. This results in the base path in the OpenAPI spec remaining /jsonapi and for all urls to be prefixed with the customized prefix.

Proposed resolution

Add support of the the jsonapi_extras module, by refactoring how the prefix is determined. We should calculate the api prefix in getBasePath() and then strip that value from all the jsonapi routes.

API changes

This makes a change the to the OpenApiGeneratorBase class in order to have complete dependency injection and not use \Drupal in the class definition. This is an acceptable change to make as the project is still alpha, and it follows coding standards. The patch resolves all instances of the service instantiation.

Original report by mpdonadio

I am seeing two problems with the Server URLs for the endpoints listed in the `admin/api` path:

In this case

http://kb.example.com/jsonapi/api/files

should be

https://kb.example.com/api/files

1. This particular server is HTTPS, but the sample path us HTTP
2. 'jsonapi' is being added to the path.

I'm assuming this is a problem in `OpenApiDocs::generateDocsFromQuery()`, but didn't see exactly what was wrong.

CommentFileSizeAuthor
#14 2897665-14-wrong-server-urls.patch11.41 KBrichgerdes
#11 2897665-11-wrong-server-urls.patch10.7 KBrichgerdes
#10 2897665-10-inter.diff10.84 KBrichgerdes
#10 2897665-10-wrong-server-urls.patch10.7 KBrichgerdes
#9 interdiff-2897665-7-9.diff.txt702 bytesszeidler
#9 2897665-9.patch1.7 KBszeidler
#8 Screen Shot 2017-09-26 at 1.41.01 PM.png20.8 KBmpdonadio
#7 interdiff-2897665.txt646 bytesdawehner
#7 2897665-7.patch1.7 KBdawehner
#6 Screen Shot 2017-09-25 at 4.10.41 PM.png20.77 KBmpdonadio
#5 2897665-5.patch1.07 KBdawehner
#4 Screen Shot 2017-09-15 at 8.20.32 AM.png20.82 KBmpdonadio
#3 Screen Shot 2017-09-15 at 00.35.15.jpg54.06 KBdawehner
28635292-0bd63fa8-7208-11e7-9fa1-8a972ab60fa6.png19.33 KBmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented

This was discovered in https://github.com/contentacms/contenta_jsonapi/issues/160

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Issue summary: View changes
FileSize
Screen Shot 2017-09-15 at 00.35.15.jpg54.06 KB

@mpdonadio I cannot longer reproduce this issue ...

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
FileSize
Screen Shot 2017-09-15 at 8.20.32 AM.png20.82 KB

Open the dropdown. On my install (which is latest):

This site is HTTPS, and it is prepending the /jsonapi

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Active » Needs review
FileSize
2897665-5.patch1.07 KB

Oh I see, sorry @mpdonadio. Thank you I was able to reproduce it. Here should be a fix for that.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Status: Needs review » Needs work
FileSize
Screen Shot 2017-09-25 at 4.10.41 PM.png20.77 KB

Path is fixed, but didn't pick up URL protocol:

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review
FileSize
2897665-7.patch1.7 KB
interdiff-2897665.txt646 bytes

Nice observation ...

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2017-09-26 at 1.41.01 PM.png20.8 KB

LGTM.

Sorry I couldn't help with this. Need to get local certs going on my dev machine.

szeidler’s picture

szeidler CreditAttribution: szeidler at Ramsalt Lab commented
Status: Reviewed & tested by the community » Needs review
FileSize
2897665-9.patch1.7 KB
interdiff-2897665-7-9.diff.txt702 bytes

Patch #7 is working fine for me, BUT I found another nitpick. The server url is not including a port, when the API is served using a non default port.

The attached patch should fix this, too.

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
CreditAttribution: richgerdes commented
Issue summary: View changes
FileSize
2897665-10-wrong-server-urls.patch10.7 KB
2897665-10-inter.diff10.84 KB

I have improved the patch in #9 to use best practices and to generate correct openapi standards. Removing the basePath should not be done, and the altered prefix (the result of using JsonApi Extras) should be set there and then stripped from all routes when creating the specifications.

Additionally, I merged the changes for handling the protocol and including the port number in the specifications, from #9 to ensure that we best support the variance in platforms as we can and removed the use of \Drupal in these files, replacing it with relevant dependency injection, to match Drupal coding standards.

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
CreditAttribution: richgerdes commented
FileSize
2897665-11-wrong-server-urls.patch10.7 KB

Fixed variable typo.

Status: Needs review » Needs work

The last submitted patch, 11: 2897665-11-wrong-server-urls.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

Thanks everyone for the work here!

After #2936116: Test fail against current core verison. the tests here still fail. Would it make sense to solve #2930810: Make tests more resilient to changes in Drupal core first?

richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
CreditAttribution: richgerdes commented
Status: Needs work » Needs review
FileSize
2897665-14-wrong-server-urls.patch11.41 KB

Tests have been reworked to pass for both 8.4 and 8.5. Re rolled the patch around the latest dev version.

  • richgerdes authored e90b7ff on 8.x-1.x 
    Issue #2897665 by richgerdes, dawehner, szeidler, mpdonadio: Wrong...
richgerdes’s picture

richgerdes
he/him
Primary language English
Location New Jersey, USA
CreditAttribution: richgerdes commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.