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

Prevent Overwriting Files with Same Slug Names #1239

Merged
merged 7 commits into from
May 23, 2018

Conversation

brianlmacdonald
Copy link
Contributor

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
screen shot 2018-04-09 at 1 26 30 pm

Saved the file successfully, and checked that it was in workflow.
screen shot 2018-04-09 at 1 27 32 pm

Then created a new file, also called test, tried to save it, and--
screen shot 2018-04-09 at 1 28 55 pm

Then instead, saved it as Test 2.
screen shot 2018-04-09 at 1 30 00 pm

Then published it.
screen shot 2018-04-09 at 1 30 19 pm

Then created a new file, also called Test 2, and tried to save it, and --
screen shot 2018-04-09 at 1 30 34 pm

Error stopped it. All files are safe.

screen shot 2018-04-09 at 1 33 35 pm

screen shot 2018-04-09 at 1 33 41 pm

- 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!
fennec_fox

@verythorough
Copy link
Contributor

verythorough commented Apr 9, 2018

Deploy preview for netlify-cms-www ready!

Built with commit 3c02916

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

@verythorough
Copy link
Contributor

verythorough commented Apr 9, 2018

Deploy preview for cms-demo ready!

Built with commit 3c02916

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

persistEntry(config, collection, entryDraft, MediaFiles, integrations, options = {}) {
async checkOverwrite(collection, slug, path, entryDraft) {
const errorMessage = "Oops, duplicate filename found. Please choose a unique name."
const existingEntry = window.repoFilesUnpublished.find(e => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@tech4him1 tech4him1 Apr 9, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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);
  }

screen shot 2018-04-09 at 3 59 43 pm

Ideas to get around this?

Copy link
Contributor

Choose a reason for hiding this comment

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

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));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tech4him1 badass!

@tech4him1
Copy link
Contributor

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

@brianlmacdonald
Copy link
Contributor Author

brianlmacdonald commented Apr 10, 2018

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

@erquhart
Copy link
Contributor

erquhart commented May 14, 2018

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
Copy link
Contributor

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

@erquhart
Copy link
Contributor

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

slug,
raw: this.entryToRaw(collection, entryDraft.get("entry")),
};
entryObj = await this.checkOverwrite(collection, slug, path, entryDraft);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@erquhart erquhart May 14, 2018

Choose a reason for hiding this comment

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

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

@erquhart
Copy link
Contributor

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.

Copy link
Contributor Author

@brianlmacdonald brianlmacdonald left a comment

Choose a reason for hiding this comment

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

Looks awesome!

return key.toLowerCase().trim() === field;
});
});
return keys.find(key => entryData.get(key) !== undefined);
Copy link
Contributor

@tech4him1 tech4him1 May 22, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@erquhart erquhart merged commit 9a1c668 into decaporg:master May 23, 2018
erquhart added a commit to erquhart/netlify-cms that referenced this pull request May 24, 2018
erquhart added a commit to erquhart/netlify-cms that referenced this pull request May 24, 2018
@erquhart
Copy link
Contributor

erquhart commented May 24, 2018

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
Copy link
Contributor Author

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

@erquhart
Copy link
Contributor

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

@brianlmacdonald
Copy link
Contributor Author

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

@brianlmacdonald
Copy link
Contributor Author

brianlmacdonald commented May 24, 2018

@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.

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.

4 participants