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.

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

404s with headless preview for unsaved drafts and disabled entries #4581

Closed
ghost opened this issue Jul 16, 2019 · 38 comments
Closed

404s with headless preview for unsaved drafts and disabled entries #4581

ghost opened this issue Jul 16, 2019 · 38 comments
Labels
enhancement improvements to existing features site development 👩‍💻 features related to website/API development

Comments

@ghost
Copy link

ghost commented Jul 16, 2019

Description

Don't know whether i missed something in my setup or that's an issue, but couldn't get a headless preview working for unsaved drafts.

So the default preview for an unsaved draft works fine with a url like

http://temp2.local/en/news/__temp_9hWhjyD2oqL6eaGscUyxndcGZ6gMksJ9cP1X?x-craft-preview=qtdgDQI8ut&token=jIUdZHOcTRKHNeA_zLjgTKoBDd0JWDdY

However trying to get a preview from a simple vue js app via element api fails with

{ "error": { "code": 404, "message": "No element exists that matches the endpoint criteria" } }

The element api url is

http://temp2.local/en/api/v1/entry/__temp_9hWhjyD2oqL6eaGscUyxndcGZ6gMksJ9cP1X?token=jIUdZHOcTRKHNeA_zLjgTKoBDd0JWDdY

The config is

'api/v1/entry/<slug:[^\/]+>' => function($slug) {
    return [
        'elementType' => Entry::class,
        'criteria' => ['slug' => $slug] ,
        'one' => true,
        'pretty' => true,
        'cache' => false,
        'transformer' => new EntryTransformer(),
    ];
},

Once the entry is saved, and a new draft is created, the headless preview for that draft works fine:

http://temp2.local/en/api/v1/entry/headless-preview?token=R2Wp36Lr_Z8Zu7utYDnv6ERNDH9a3zWP gets the correct draft data.

When the source entry is disabled or the post date is set to a future date, the default preview for a new draft works, but the headless version fails again with 404.

Additional info

  • Craft version: 3.2.2
  • PHP version: 7.2.14
  • Database driver & version: MySQL 5.7.14
  • Plugins & versions: element-api 2.5.4
@brandonkelly
Copy link
Member

brandonkelly commented Jul 16, 2019

Hm, yeah, that’s a little tricky. Element API is just generating an element query based on the criteria you passed, and if the entry isn’t fully saved yet, then it’s not going to show up in element query results, unless the drafts param is set.

You could work around it by doing this:

  1. Add ?isDraft={isDraft} to your Preview Target URL (assuming you are using a custom Preview Target for this).

  2. In your JS, pass along the isDraft query param to your Element API request, alongside the token param. So the full endpoint URL would end up looking like this in the event that it’s a draft:

    http://temp2.local/en/api/v1/entry/__temp_9hWhjyD2oqL6eaGscUyxndcGZ6gMksJ9cP1X?token=jIUdZHOcTRKHNeA_zLjgTKoBDd0JWDdY&isDraft=1
    
  3. In your endpoint config, change your criteria to this:

    'criteria' => [
        'slug' => $slug,
        'drafts' => (bool)Craft::$app->request->getQueryParam('isDraft'),
    ],

Let me know if that works for you.

@ghost
Copy link
Author

ghost commented Jul 17, 2019

@brandonkelly Ah thanks, i somehow thought the token stuff would be completely handled by Craft core.

So: yes that worked, but with a few quick tests i saw (maybe incorrectly) a couple of problems:

  • didn't work for pending entries, so a param like status=>null had to be added
  • needs a corresponding way for revisions
  • picked up changes to the slug only with a page reload
  • you could completely omit the token from the api url, so gaining unauthorized access.

So i'm going to try something like this to get the criteria from the token (yet not fully tested and only semi understood):

public static function getTokenCriteria(array $criteria): array
{

    $token = Craft::$app->request->getParam('token');
    if (!$token) {
        return $criteria;
    }

    $tokenRoute = Craft::$app->tokens->getTokenRoute($token);
    if (!$tokenRoute) {
        throw new NotFoundHttpException();
    }
    $params = $tokenRoute[1];
    $criteria = [
        'status' => null,
        'siteId' => $params['siteId']
    ];

    if ($params['draftId']) {
        $criteria['drafts'] = true;
        $criteria['draftId'] = $params['draftId'];
    } elseif ($params['revisionId']) {
        $criteria['revisions'] = true;
        $criteria['revisionId'] = $params['revisionId'];
    } else {
        $criteria['id'] = $params['sourceId'];
    }

    return $criteria;
}

Do you think that's a way to approach it?

@brandonkelly
Copy link
Member

Yeah maybe. I’m going to think on this as well, and see if there’s something we can do to make it easier.

@ghost
Copy link
Author

ghost commented Jul 17, 2019

@brandonkelly Thanks, just finished some kind of Proof of Concept with this, which worked well (with some very impressed clients, this one is a real game changer), so it's ok for me right now , but yes, it would be nice to have some consistent behavior between twig and headless previews, especially thinking of previewing not only single entry views, but also some kind of list/index views.

@brandonkelly brandonkelly reopened this Jul 18, 2019
@brandonkelly
Copy link
Member

Sweet!

@brandonkelly
Copy link
Member

brandonkelly commented Jul 18, 2019

This should be a lot easier starting in Craft 3.2.5, as element queries will now include the previewed elements in the results even if the status / drafts / revisions params are set to exclude them.

To test this change early, change your craftcms/cms requirement in composer.json to:

"require": {
  "craftcms/cms": "dev-develop#621c751b3a5ee404a7111faed3839303fa81caf5 as 3.2.4.1",
  "...": "..."
}

Then run composer update, and then you should be able to remove all of your code that worked around this issue with unsaved drafts & disabled entries.

@narration-sd
Copy link
Contributor

narration-sd commented Jul 18, 2019

Ok! Caught just before out the door, so I tried it. And, at first look at least it works: previews show again on New entry, in Live Vue on Gatsby.

However, one issue: the new Entry apparently has a null for the Title field, and CraftQL/or probably the actual GraphQL engine it fronts for doesn't like this.

Simply putting an empty string into title of your created Entry should fix this, as other text fields appear to dummy up this way automatically.

Well, I have to test that, will have to be later, as there's only a Redactor field in my quick runup. But you get the idea, and can fix if simpler text fields also might not be defaulting to non-null...

Here's how Hal sees this error: in green...

hal-on-null-title

@narration-sd
Copy link
Contributor

Und...I tested with a plain text field added to the Entrytype schema, and this is also fine.

Actually in reading more carefully the Hal/CraftQL error in pic above, it's a little clearer about the exact problem, I suppose. Titles aren't allowed to be null by constraint. Thus again the blank title needs to be assigned for your dummy Entry, seems.

Which made me think of another possible issue, so tested that also. I made the new test simple text field required. But this did not cause a problem, as probably evident to you, because required is a constraint only on saving.

So, the faulting titles constraint is earlier, probably on the database, and satisfying it so that the title isn't null may be all that's needed to have this upgrade become fully functional. Cheers.

@brandonkelly
Copy link
Member

Just fixed a bug caused by that previous commit, where previewing normal entry drafts could give you duplicate query results. Updated the composer.json code with the new commit hash.

@narration-sd This seems like something the GraphQL API should just be able to work around, considering not all element types must have titles (which is why it can be stored as null to begin with), and even when titles are required, if none has been entered yet I think null is the expected value.

@ghost
Copy link
Author

ghost commented Jul 19, 2019

@brandonkelly Thanks, with just a few tests that looks great, both for single entry and index views. Will do a few more later on.

Not quite sure though what is the best idea of building headless routes in this scenario.

Currently using something like /client/app#/{site.handle}/news/{slug} . When changing the slug, i ran into some situations where the headless preview 404s, using the 'wrong' version of the slug (without having analyzed that in detail). Maybe it's better to always include the id (sourceId i guess?) in the headless route and pass that to the element-api call.

@brandonkelly
Copy link
Member

Yeah I think the ID is generally going to be safer; that’s what I’ve been using locally.

@narration-sd
Copy link
Contributor

narration-sd commented Jul 19, 2019

@narration-sd This seems like something the GraphQL API should just be able to work around, considering not all element types must have titles (which is why it can be stored as null to begin with), and even when titles are required, if none has been entered yet I think null is the expected value.

Yeah, you'd think so. This is one of several reasons I ran a retest with a simple Text field 'just like' Title. CraftQL returns a null fine for that as well as any more complex field I've ever tried.

So it has to be something special about Title, and for that, it seems it must be picking something up from Craft in its GQL schema building which marks Title so. Tracing the layers of set theory that monster is coded of is no fun, of course.

I am a little curious that this isn't a general problem for you that I've found?

@narration-sd
Copy link
Contributor

narration-sd commented Jul 19, 2019

amplifying last point: have you tried Previewing a fresh empty New entry???

@brandonkelly
Copy link
Member

I’ve been testing with the following template:

<!DOCTYPE html>
<html lang="en-US">
<title>Headless Preview Test</title>
<script src="https://code.jquery.com/jquery-3.4.1.min.js"></script>
<script>
  $.get('/api/entry/{{ craft.app.request.getQueryParam('id') }}.json?token={{ craft.app.request.getQueryParam('token') }}', function(response) {
      $('<pre/>', {
          html: JSON.stringify(response, null, 2),
      }).appendTo(document.body);
  });
</script>

Which points to the following Element API endpoint:

'api/entry/<id:\d+>.json' => function($id) {
    return [
        'elementType' => Entry::class,
        'criteria' => ['id' => $id],
        'one' => true,
        'paginate' => false,
        'transformer' => function(Entry $entry) {
            return ['title' => $entry->title];
        },
    ];
},

And I get this preview output when creating a new entry (before it’s fully saved):

{
  "title": null
}

@narration-sd
Copy link
Contributor

narration-sd commented Jul 19, 2019

Well, that’s Element API.

It’s CraftQL that’s having the problem here...

And indeed, CQL code says the message is about a '_definition.GraphQLNonNull'' type.

Of course it's into the weeds for where that would be set

@narration-sd
Copy link
Contributor

that's CQL = CraftQL; fixed it

@narration-sd
Copy link
Contributor

...and yes, he's doing it, straight up and all on his own, not asking Craft.
Line 18, vendor/markhuot/craftql/src/Types/EntryInterface.php

       $this->addStringField('title')->nonNull();

I guess because 'we know' titles can't be empty -- wasn't this true at one past time if memory serves?

And since he's using this same Schema created for upserts, then he'd want to guard.

Getting this changed -- seems likely to be a drawn-out process. Any reason not to just put an empty string into your created New entry, as asked?? Since emptiness is fine if null is? And then we can be on the air. I'm all wired up to dev-master and can test it....

@narration-sd
Copy link
Contributor

narration-sd commented Jul 19, 2019

p.s. here's a little more of the code in this area of CraftQL -- the non-null assumption is made on some other fields people are likely enough to take an interest in and query, so they should probably get the same gets-empty-string treatment. Have a glance for ->nonNull() ...

class EntryInterface extends InterfaceBuilder {

    function boot() {
        $this->addIntField('id')->nonNull();

        if ($this->request->token()->can('query:entry.author')) {
            $this->addField('author')->type(User::class)->nonNull();
        }

        $this->addStringField('title')->nonNull();
        $this->addStringField('slug')->nonNull();
        $this->addDateField('dateCreated')->nonNull();
        $this->addDateField('dateUpdated')->nonNull();
        $this->addDateField('expiryDate');
        $this->addDateField('postDate');
        $this->addBooleanField('enabled')->nonNull();
        $this->addStringField('status')->nonNull();
        $this->addStringField('uri');
        $this->addStringField('url');

        if ($this->request->token()->can('query:sections')) {
            $this->addField('section')->type(Section::class);
            $this->addField('type')->type(EntryType::class);
        }

        $this->addField('ancestors')->lists()->type(EntryInterface::class)
            ->resolve(function ($root) {
                return $root->getAncestors()->all();
            });

        $this->addField('children')
            ->lists()
            ->type(EntryInterface::class)
            ->use(new EntryQueryArguments)
            ->resolve(function ($root, $args, $context, $info) {
                return $this->request->entries($root->{$info->fieldName}, $root, $args, $context, $info)->all();
            });

        $this->addField('descendants')->lists()->type(EntryInterface::class)
            ->resolve(function ($root) {
                return $root->getDescendants()->all();
            });

        $this->addBooleanField('hasDescendants')->nonNull();
        $this->addIntField('level');

        $this->addField('parent')->type(EntryInterface::class);

        $this->addField('siblings')->lists()->type(EntryInterface::class)
            ->resolve(function ($root) {
                return $root->getSiblings()->all();
            });
    }
...
}

@narration-sd
Copy link
Contributor

but if you want a pre-validation, happy to test with just <title> preset to empty string, to prove the cause on a one-liner for you in dev-master...

@narration-sd
Copy link
Contributor

narration-sd commented Jul 21, 2019

...and...yes, there's a further problem here.

I was thinking you're operating pretty far into overload, so why not a) provide a temp fix inside Live Vue as I have to degree possible (impossible fully...) for the not-yet-accepted PR on other issue b) cook up a pre-PR that at least shows where a real solution may be placed. But on the b) part....

  • If I put an empty string into title directly into the database using Navicat during an appropriate xdebug break, the entry becomes acceptable (for query asking for title at least).
  • however, if I do that in code where you create the entry and save it, the result is still a null in the database, and again CraftQL will fail the query and thus the preview.

[ok so far, but I think have found the actual problem, and it's not an option setting on MySql after all, which is what followed but now redacting...see next +1 below]

@narration-sd
Copy link
Contributor

Just to add -- do your own analysis to assure, please -- but I took another weekend moment to list out and check the returned new entry for the ->nonNull() status properties I see in Mark's code.

Result: it looks like title is actually the only one not set for these fresh New entries, and thus needing to end up with an empty string in it.

@narration-sd
Copy link
Contributor

narration-sd commented Jul 21, 2019

Well, think I've located where this null thing is happening. See line 172-174 of services/Content.php:

        if ($element->hasTitles() && ($title = (string)$element->title) !== '') {
            $values['title'] = $title;
        }

The result of this for an empty-string title is not to set -- and thus the ddl default for title will be used, which is...null.

Lots of opportunity to think what's least intrusive to fix this, and only your breadth of knowledge about consequences can decide, @brandonkelly, so no PR.

If you're not using null title as a flag somewhere, maybe just allowing empty string here could make it simple to push an empty string around line 96 of EntryController::actionCreateDraft where I tried it, but I somehow doubt it's that straightforward.

Anyway, hope it's been useful to have run these things down, towards getting this solved.

@narration-sd
Copy link
Contributor

It did occur to me for a moment falling asleep last night that the easiest/cleanest way to fix this might be just to switch that condition and action around, just mentioned on line 172 of services/Content.php.

So that IF title IS NULL THEN title = '' ENDIf

Seems this would have the added advantage of avoiding need for special case on title === null in areas like searches etc. who could use it....

brandonkelly added a commit that referenced this issue Jul 22, 2019
@brandonkelly
Copy link
Member

@narration-sd I don’t want to stop storing element titles as null if it’s blank; however it occurred to me that I could swap it out at the element query level in a way that is relatively not hacky. (4909d95)

@narration-sd
Copy link
Contributor

Great, that's the other end, and.

I swapped in on the hash, ran things up, and...it works.

CraftQL is now happy again, and Live Vue has previews and shares on fresh New Entries. Besides the rest.

Thanks, @brandonkelly , looks like last loose board on this roofing is now nailed down.

@mattandrews
Copy link

@brandonkelly apologies to dig this back up, but we're still seeing issues with this in Craft 3.3.6.

If I make a new draft entry (and don't save it) and attempt to live preview it, my backend makes an call to my Element API with the relevant x-craft-live-preview and token parameters. My entry query looks for a slug (like @wsydney76 does here) which fails as the slug is __temp_r3YeftAf2HADFWbhCXOcZT8lNS5zThDwx8rL.

I don't want to swap my frontend URLs to use IDs rather than slugs – is there any way I can work around this? If I disable the $criteria['slug'] = $slug; logic when the token param is set then everything works, but then "normal" (eg. published) entries don't preview properly (a different entry gets loaded?!).

@brandonkelly
Copy link
Member

@mattandrews Can you post your endpoint’s full criteria definition?

@mattandrews
Copy link

Yep, sorry! Here's what I'm using on a typical query:

Array
(
    [site] => en
    [section] => updates
    [status] => Array
        (
            [0] => live
            [1] => expired
        )

    [type] => press_releases
    [slug] => 25th-birthday-launch
)

(We allow expired entries for boring internal reasons)

@brandonkelly
Copy link
Member

And how are you passing your slug to the JS? Does your preview target URL contain &slug={slug} or something?

@mattandrews
Copy link

So my frontend URL for the above query looks like /press_releases/2019-12-10/25th-birthday-launch, and we have some server-side code (no JS!) which passes that entire string onto the Element API, which uses the type and slug parameters to build the above criteria. There's no preview targets configured beyond this:

image

@brandonkelly
Copy link
Member

and we have some server-side code (no JS!) which passes that entire string onto the Element API, which uses the type and slug parameters to build the above criteria.

Can you post that code?

@mattandrews
Copy link

mattandrews commented Dec 20, 2019

Sure. This is probably a bit of a deep-dive, apologies in advance:

The "Updates" section has this URL config:
image
(the replace function there is a bit of hackery to allow us to have an entry type handle called "press_releases" which becomes "press-releases" in URLs – we end up with a URL like /news/press-releases/2019-12-20/slug-goes-here).

Our NodeJS frontend app receives these pageviews and calls our Element API here. The relevant parts are:

function addPreviewParams(requestParams = {}, params = {}) {
    const globalParams = pick(requestParams, [
        'social',
        'x-craft-live-preview',
        'x-craft-preview',
        'token'
    ]);
    return Object.assign({}, globalParams, params);
}

return fetch(`/v1/${locale}/updates/${type}/${date}/${slug}`, {
    qs: addPreviewParams(requestParams, { ...query })
}).then(response => {
    return {
        meta: response.meta,
        result: response.data.attributes
    };
});

The addPreviewParams() function just picks some whitelisted URL parameters and merges them with any others passed in. I won't dive into where requestParams and query are set but when I log them out locally, this is what they're defined as:

requestParams (eg. Craft-related tokens)

{
  'x-craft-live-preview': 'tviI56JIF6',
  'token': 'sCKNEA9ho6MwIoMtzHaGzSZJTl60fbL9'
}

(query is used for things like pagination, though not used in this call)

All of this builds a URL which looks like this:

/v1/en/updates/press-releases/2019-12-20/__temp_Bic3Qw6MHnr4Pfrk6H1FfYPsuHff96aelVef?x-craft-live-preview=tviI56JIF6&token=sCKNEA9ho6MwIoMtzHaGzSZJTl60fbL9

Now it's over to our PHP endpoint for the Element API.

Here's a simplified version of the above code showing just the relevant parts:


function isDraftOrVersion()
{
    $isDraft = \Craft::$app->request->getParam('draft');
    $isVersion = \Craft::$app->request->getParam('version');
    return $isDraft || $isVersion;
}

function getVersionStatuses()
{
 
    $statuses = ['live', 'expired'];
    if (isDraftOrVersion()) {
        $statuses[] = 'disabled';
    }
    return $statuses;
}

function getUpdates($locale, $type = null, $date = null, $slug = null)
{
    $isSinglePost = $date && $slug;
    $defaultPageLimit = 10;
    $pageLimit = \Craft::$app->request->getParam('page-limit') ?: $defaultPageLimit;
    $criteria = [
        'site' => $locale,
        'section' => 'updates',
        'status' => getVersionStatuses(),
    ];
    if ($type) {
        $criteria['type'] = str_replace('-', '_', $type);
    }
    if ($isSinglePost) {
        $criteria['slug'] = $slug;
    } else {
        // <SNIP> a bunch of logic for handling listing pages etc
    }
    return [
        'serializer' => 'jsonApi',
        'elementType' => Entry::class,
        'criteria' => $criteria,
        'resourceKey' => 'updates',
        'elementsPerPage' => $isSinglePost ? null : $pageLimit,
        'one' => $isSinglePost,
        'transformer' => new UpdatesTransformer($locale),
    ];
}

That API call returns the following:

{"error":{"code":404,"message":"No element exists that matches the endpoint criteria"}}

If I remove this line:

$criteria['slug'] = $slug;

... then it works when trying to preview unsaved drafts, but if I preview a new draft of an existing entry, it shows me content from a different entry (looks like the newest item with the same Entry Type as the one I'm editing).

Hope this helps and apologies if this ends up just being some obvious misconfiguration on my part.

@brandonkelly
Copy link
Member

brandonkelly commented Dec 20, 2019

Hah ok… I just tested this locally and it’s definitely still working, so something must be getting lost along the way here.

Here’s the endpoint config I used to test:

'test/<slug:[^\/]*>.json' => function(string $slug) {
    return [
        'elementType' => Entry::class,
        'criteria' => [
            'section' => 'news',
            'slug' => $slug,
        ],
        'one' => true,
        'pretty' => true,
        'transformer' => function(Entry $entry) {
            return [
                'title' => $entry->title,
                'slug' => $entry->slug,
                'id' => $entry->id,
            ];
        },
    ];
},

I’m pinging that directly via a preview target, with the URL Format test/{slug}.json. And that is working correctly regardless of whether the entry is brand new (“unsaved”) or existing.

@mattandrews
Copy link

Aha, I think we're onto something. I tried using your code above and still had no luck (eg. it's not something weird about how I'm setting the endpoint criteria). The key was your comment "via a preview target".

I'm just using a regular entry URL config, no preview targets. But I just tried adding a preview target for this section using the exact same URL config, and it worked:

image

Now when I attempt a live preview, I get the same 404 error as before for the "Primary Entry" but if I toggle to "Matt test" preview target, I see my preview page as expected.

The URLs of the preview iframe are:

Primary Entry:

/news/press-releases/2020-01-08/__temp_dUnA9qgivP4erwVhRk0K3SybyxlWTRngHCeV?x-craft-live-preview=ZVDVuU8c0w&token=BvLYISk2x4NpQ7uHXT13n-R143020Q68

Preview target

/news/press-releases/2020-01-08/matt-test-press-release-2020?x-craft-live-preview=v3iFrmax6A&token=BvLYISk2x4NpQ7uHXT13n-R143020Q68

So possibly something isn't working here with the slug where it's being slugified from my Title field for preview targets, but not for primary entry URIs?

@brandonkelly
Copy link
Member

brandonkelly commented Jan 9, 2020

Aha! That makes perfect sense, because Craft explicitly avoids updating an entry draft’s URI, so that it will always match the source entry – and if a URI is to change, that will only happen once the draft is actually published.

// If this is a draft or revision and it already has a URI, leave it alone
/** @var Element $model */
if (($model->getIsDraft() || $model->getIsRevision()) && $model->uri) {
return;
}

Though I think we can tweak that condition a bit, to ignore unsaved drafts (the state that entries are in before you click “Save” for the first time). Because in their case, there is no source entry whose URI the draft needs to try to stay consistent with.

@brandonkelly brandonkelly reopened this Jan 9, 2020
@brandonkelly brandonkelly added enhancement improvements to existing features site development 👩‍💻 features related to website/API development labels Jan 9, 2020
@brandonkelly brandonkelly added this to the 3.4 milestone Jan 9, 2020
brandonkelly added a commit that referenced this issue Jan 9, 2020
@brandonkelly
Copy link
Member

@mattandrews just tweaked this behavior for Craft 3.4, so going forward, unsaved entry URIs will be updated on each autosave.

@mattandrews
Copy link

And this is why I love Craft. Thanks so much for the quick fix (and taking the time go through this with everyone here), @brandonkelly!

@brandonkelly
Copy link
Member

My pleasure. Thanks for identifying the bug!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvements to existing features site development 👩‍💻 features related to website/API development
Projects
None yet
Development

No branches or pull requests

3 participants