Prevent Overwriting Files with Same Slug Names

[brianlmacdonald]

Closes #1164.

Previously, when a file was added with the same title as an existing one and the slug equals title, the existing file is overwritten.

- Summary

This checks in src/backends/backend.js before the entryObj is created in persistEntry if there's an existing file with the same name. Throws an Error if there is.

First, for editorialWorkflow mode, it checks in window.repoFilesUnpublished for preexisting entry. Next, it uses this.implementation.getEntry, checking if there's already a published entry in danger of being overwritten.

- Test plan
I created a file called test

Saved the file successfully, and checked that it was in workflow.

Then created a new file, also called test, tried to save it, and--

Then instead, saved it as Test 2.

Then published it.

Then created a new file, also called Test 2, and tried to save it, and --

Error stopped it. All files are safe.

- Description for the changelog

Prevents file overwrite when encountering slug name collisions.

- A picture of a cute animal (not mandatory but encouraged)
Behold, a Fennec Fox at rest!

[verythorough]

Deploy preview for netlify-cms-www ready!

Built with commit 3c02916

https://deploy-preview-1239--netlify-cms-www.netlify.com

[verythorough]

Deploy preview for cms-demo ready!

Built with commit 3c02916

https://deploy-preview-1239--cms-demo.netlify.com

[tech4him1]

Unfortunately, repoFilesUnpublished is an implementation detail of the test-repo backend, not something most backend use, so this will have to be checked using a different method.

[tech4him1]

To get them for any backend type, you should be able to use backend.unpublishedEntries().

[tech4him1]

It may be easier and more performant, however, to just check for the specific entry using unpublishedEntry() instead of filtering them yourself.

[brianlmacdonald]

@tech4him1 the problem is when backend.unpublishedEntry uses implementation.unpublishedEntry it throws an error if there is no entry found.

unpublishedEntry(collection, slug) {
    const entry = window.repoFilesUnpublished.find(e => (
      e.metaData.collection === collection.get('name') && e.slug === slug
    ));
    if (!entry) {
      return Promise.reject(new EditorialWorkflowError('content is not under editorial workflow', true));
    }
    return Promise.resolve(entry);
  }

Ideas to get around this?

[tech4him1]

What about:

import { EditorialWorkflowError } from "ValueObjects/errors"; // at top of js

const existingEntry = await this.implementation.unpublishedEntry(...).catch(error => ((error instanceof EditorialWorkflowError && error.notUnderEditorialWorkflow) ? Promise.resolve(false) : Promise.reject(error));

[brianlmacdonald]

@tech4him1 badass!

[tech4him1]

@brianlmacdonald Do you mind if I rebase this so the commit history and diff is clean?

[brianlmacdonald]

@tech4him1 Ugh. Sorry, when the fork branch and local branch diverged is where I failed to rebase this time.

[erquhart]

With explicit slug editing on the horizon, I'm now thinking the goals of this PR should be met as a part of slug validation.

Edit: hmm I was seeing some issues in the PR that made me think it simpler to take a different route, but on further review this PR seems to satisfy it's goal for now.

[tech4him1]

@erquhart I always assumed this was intended to be a stop-gap until we explicit slug editing was added?

[erquhart]

Yeah, see my edit. I'm thinking this is good to go.

[erquhart]

Instead of implicitly constructing and returning the entry object from checkOverwrite, we should construct the entry object here either before or after the check.

[erquhart]

Working on an update commit. Noticed this only covers the "title" field, while both "title" and "field" could be used for the slug.

[erquhart]

Added a commit that does two things:

1. References the identifier field reusing the same logic as the slug formatter that writes the identifiers, rather than only checking the "title" field
2. References the identifier field's label in an extended error message to ensure the user knows which field needs to be changed and why

@brianlmacdonald @tech4him1 if both of you can give a review we can get this merged.

[brianlmacdonald]

Looks awesome!

[tech4him1]

Is there a reason we can't use the old logic here, where we return the values (on line 48), instead of having to run entryData.get again here? Maybe I'm mis-reading the code.

[tech4him1]

const keyVals = validIdentifierFields.map(field => {
  return entryData.find((_, key) => {
    return key.toLowerCase().trim() === field;
  });
});
return keyVals.find(key => key !== undefined);

[erquhart]

Agreed, I believe I was thinking we were lowercasing and trimming the return value for some reason, but that isn't the case.

[erquhart]

Reverted in 1.8.2 to fix #1377. @brianlmacdonald this was breaking both simple and editorial workflow publishing of new entries. If you want to give it another go please open a new PR.

[brianlmacdonald]

@erquhart sure. Was this working before because it was running through the test-repo?

[erquhart]

Could have sworn I tested against real backends, but maybe it was just the test repo.

[brianlmacdonald]

I'm making a github backend now and running locally. I'm on gitter, I'll fire off questions if I get stuck.

[brianlmacdonald]

@erquhart This ended up being the fix. getEntry when in the gitHub implementation returned a 404 and we weren't accounting for it. I was able to successfully test this with a github backend.

const publishedEntry = await this.implementation.getEntry(collection, slug, path)
      .then(({ data }) => data)
      **.catch(error => {
        if (error.message === 'Not Found') {
          return Promise.resolve(false);
        }
        return Promise.reject(error);**
      });

Before I make a new PR, the git-gateway implementation doesn't have the getEntry method. Am I correct in thinking that it uses the getEntry method from github's implementation?
NM. Saw it extends it. Putting in a PR.