Commit Graph

28 Commits

Author SHA1 Message Date
Katharina Irrgang
c6a95c6478
Sorted out the mixed usages of include and withRelated (#9425)
no issue

- this commit cleans up the usages of `include` and `withRelated`.

### API layer (`include`)
- as request parameter e.g. `?include=roles,tags`
- as theme API parameter e.g. `{{get .... include="author"}}`
- as internal API access e.g. `api.posts.browse({include: 'author,tags'})`
- the `include` notation is more readable than `withRelated`
- and it allows us to use a different easier format (comma separated list)
- the API utility transforms these more readable properties into model style (or into Ghost style)

### Model access (`withRelated`)
- e.g. `models.Post.findPage({withRelated: ['tags']})`
- driven by bookshelf

---

Commits explained.

* Reorder the usage of `convertOptions`

- 1. validation
- 2. options convertion
- 3. permissions
- the reason is simple, the permission layer access the model layer
  - we have to prepare the options before talking to the model layer
- added `convertOptions` where it was missed (not required, but for consistency reasons)

* Use `withRelated` when accessing the model layer and use `include` when accessing the API layer

* Change `convertOptions` API utiliy

- API Usage
  - ghost.api(..., {include: 'tags,authors'})
  - `include` should only be used when calling the API (either via request or via manual usage)
  - `include` is only for readability and easier format
- Ghost (Model Layer Usage)
  - models.Post.findOne(..., {withRelated: ['tags', 'authors']})
  - should only use `withRelated`
  - model layer cannot read 'tags,authors`
  - model layer has no idea what `include` means, speaks a different language
  - `withRelated` is bookshelf
  - internal usage

* include-count plugin: use `withRelated` instead of `include`

- imagine you outsource this plugin to git and publish it to npm
- `include` is an unknown option in bookshelf

* Updated `permittedOptions` in base model

- `include` is no longer a known option

* Remove all occurances of `include` in the model layer

* Extend `filterOptions` base function

- this function should be called as first action
- we clone the unfiltered options
- check if you are using `include` (this is a protection which could help us in the beginning)
- check for permitted and (later on default `withRelated`) options
- the usage is coming in next commit

* Ensure we call `filterOptions` as first action

- use `ghostBookshelf.Model.filterOptions` as first action
- consistent naming pattern for incoming options: `unfilteredOptions`
- re-added allowed options for `toJSON`
- one unsolved architecture problem:
  - if you override a function e.g. `edit`
  - then you should call `filterOptions` as first action
  - the base implementation of e.g. `edit` will call it again
  - future improvement

* Removed `findOne` from Invite model

- no longer needed, the base implementation is the same
2018-02-15 10:53:53 +01:00
Katharina Irrgang
9ede5905f6
Reduce toJSON implementation: use the power of bookshelf (#9423)
refs #6103

- simplify `toJSON`
- `baseKey` was not used - have not find a single use case
- all the functionality of our `toJSON` is offered in bookshelf
- `omitPivot` does remove pivot elements from the JSON obj (bookshelf feature)
- `shallow` allows you to not return relations
- make use of `serialize`, see http://bookshelfjs.org/docs/src_base_model.js.html#line260
- fetching nested relations e.g. `users.roles` still works (unrelated to this refactoring)

> pick('shallow', 'baseKey', 'include', 'context')

We will re-add options validation in https://github.com/TryGhost/Ghost/pull/9427, but then with the official way: use `filterOptions`.

---

We return all fetched relations (pre-defined with `withRelated`) by default.
You can disable it with `shallow:true`.
2018-02-14 17:32:11 +01:00
kirrg001
fe461da110 Deleted bookshelf access plugin
refs #9127

- permission checks can happen everywhere in the code base
  - we would like to create a context class
- global access to `options.context.is(...)`
- please read more about the access plugin in #9127 section "Model layer and the access plugin".
- removed the plugin and use direct context checks
2018-01-25 17:54:28 +01:00
kirrg001
6f6c8f4521 Import lib/common only
refs #9178

- avoid importing 4 modules (logging, errors, events and i18n)
- simply require common in each file
2017-12-12 10:28:13 +01:00
kirrg001
ac2578b419 Moved errors,logging,i18n and events to lib/common
refs #9178
2017-12-12 10:28:13 +01:00
Katharina Irrgang
76689ecbee 🐛 Fixed pagination error (#9243)
no issue

- see explanation https://github.com/TryGhost/Ghost/pull/8129#issuecomment-286061529
- we catch the db error if it occurs, we can't simply check the length of the page param, because sqlite/mysql behaves differently
2017-11-14 12:47:58 +00:00
Hannah Wolfe
5319fd4e35 Added concept of ghost-query debug mode
- Outputting all queries is too much debug info for normal dev
- Use DEBUG=ghost:*,ghost-query to debug queries
- Or just DEBUG=ghost-query
2017-11-01 15:18:53 +00:00
Hannah Wolfe
bcf5a1bc34
Switch to Eslint (#9197)
refs #9178

* Add eslint deps, remove old lint deps
* Add eslint config, remove old lint configs
* Config for server and tests are different
* Tweaked rules to suit us
* Fix linting in codebase - lots of indent changes.
* Fix a real broken test
2017-11-01 13:44:54 +00:00
Hannah Wolfe
7999c38fa2 Support filtering based on primary_tag (#9124)
closes #8668, refs #8920

- Updated tests to include internal tags
  - Tests had no example of an internal tag
  - Need this to show that the new filtering works as expected
- primary_tag is a calculated field
- This ensures that we can alias the field to equivalent logic in API filters
- By replacing primary_tag by a lookup based on a tag which has order 0
- bump ghost-gql to 0.0.8

**NOTE:**
Until GQL is refactored, there are limitations on what else can be filtered when using primary_tag in a filter e.g. it wont be possible to do a filter based on primary_tag AND/OR other tag filters.
2017-10-10 14:07:44 +02:00
Katharina Irrgang
30e790bf12 Debug: Update Collision (#9103)
refs #8969

- we would like to figure out how often people get the error and with which context
2017-10-05 12:24:21 +01:00
Hannah Wolfe
512808e8b4 🐛 Added 409 UpdateCollisionError for the editor (#8899)
fixes #8898

- This is a user error, not a system error
- Downgrading to a 4xx status code means it doesn't appear in logs where it shouldn't
- We didn't have a suitable error available so I added UpdateCollisionError with 409 status
2017-08-15 12:06:40 +02:00
Katharina Irrgang
37e28cb6ef 🐛 fix updated_at is null (#8434)
closes #8426

- if you import posts with updated_at=null, you are not able to save this post anymore
- i am not sure how this is even possible, but maybe there is a case where updated_at can be null
2017-05-12 12:36:26 +01:00
Katharina Irrgang
c93f03b87e post update collision detection (#8328) (#8362)
closes #5599

If two users edit the same post, it can happen that they override each others content or post settings. With this change this won't happen anymore.

 Update collision for posts
- add a new bookshelf plugin to detect these changes
- use the `changed` object of bookshelf -> we don't have to create our own diff
- compare client and server updated_at field
- run editing posts in a transaction (see comments in code base)

🙀  update collision for tags
- `updateTags` for adding posts on `onCreated` - happens after the post was inserted
   --> it's "okay" to attach the tags afterwards on insert
   --> there is no need to add collision for inserting data
   --> it's very hard to move the updateTags call to `onCreating`, because the `updateTags` function queries the database to look up the affected post
- `updateTags` while editing posts on `onSaving` - all operations run in a transactions and are rolled back if something get's rejected

- Post model edit: if we push a transaction from outside, take this one

  introduce options.forUpdate
- if two queries happening in a transaction we have to signalise knex/mysql that we select for an update
- otherwise the following case happens:
  >> you fetch posts for an update
  >> a user requests comes in and updates the post (e.g. sets title to "X")
  >> you update the fetched posts, title would get overriden to the old one

use options.forUpdate and protect internal post updates: model listeners
- use a transaction for listener updates
- signalise forUpdate
- write a complex test

use options.forUpdate and protect internal post updates: scheduling
- publish endpoint runs in a transaction
- add complex test
- @TODO: right now scheduling api uses posts api, therefor we had to extend the options for api's
  >> allowed to pass transactions through it
  >> but these are only allowed if defined from outside {opts: [...]}
  >> so i think this is fine and not dirty
  >> will wait for opinions
  >> alternatively we have to re-write the scheduling endpoint to use the models directly
2017-04-19 14:53:23 +01:00
Ben Vibhagool
ad9b59c87c Fix access-rules plugin description comment (#7665)
no issue

The plugin extends `Bookshelf.Model.forge` not` Bookshelf.Model.force`
2016-11-02 12:40:09 +01:00
Katharina Irrgang
d81bc91bd2 Error creation (#7477)
refs #7116, refs #2001

- Changes the way Ghost errors are implemented to benefit from proper inheritance
- Moves all error definitions into a single file
- Changes the error constructor to take an options object, rather than needing the arguments to be passed in the correct order.
- Provides a wrapper so that any errors that haven't already been converted to GhostErrors get converted before they are displayed.

Summary of changes:

* 🐛  set NODE_ENV in config handler
*   add GhostError implementation (core/server/errors.js)
  - register all errors in one file
  - inheritance from GhostError
  - option pattern
* 🔥  remove all error files
*   wrap all errors into GhostError in case of HTTP
* 🎨  adaptions
  - option pattern for errors
  - use GhostError when needed
* 🎨  revert debug deletion and add TODO for error id's
2016-10-06 13:27:35 +01:00
Katharina Irrgang
1882278b5b 🎨 configurable logging with bunyan (#7431)
- 🛠  add bunyan and prettyjson, remove morgan

-   add logging module
  - GhostLogger class that handles setup of bunyan
  - PrettyStream for stdout

-   config for logging
  - @TODO: testing level fatal?

-   log each request via GhostLogger (express middleware)
  - @TODO: add errors to output

- 🔥  remove errors.updateActiveTheme
  - we can read the value from config

- 🔥  remove 15 helper functions in core/server/errors/index.js
  - all these functions get replaced by modules:
    1. logging
    2. error middleware handling for html/json
    3. error creation (which will be part of PR #7477)

-   add express error handler for html/json
  - one true error handler for express responses
  - contains still some TODO's, but they are not high priority for first implementation/integration
  - this middleware only takes responsibility of either rendering html responses or return json error responses

- 🎨  use new express error handler in middleware/index
  - 404 and 500 handling

- 🎨  return error instead of error message in permissions/index.js
  - the rule for error handling should be: if you call a unit, this unit should return a custom Ghost error

- 🎨  wrap serve static module
  - rule: if you call a module/unit, you should always wrap this error
  - it's always the same rule
  - so the caller never has to worry about what comes back
  - it's always a clear error instance
  - in this case: we return our notfounderror if serve static does not find the resource
  - this avoid having checks everywhere

- 🎨  replace usages of errors/index.js functions and adapt tests
  - use logging.error, logging.warn
  - make tests green
  - remove some usages of logging and throwing api errors -> because when a request is involved, logging happens automatically

- 🐛  return errorDetails to Ghost-Admin
  - errorDetails is used for Theme error handling

- 🎨  use 500er error for theme is missing error in theme-handler

- 🎨  extend file rotation to 1w
2016-10-04 16:33:43 +01:00
Katharina Irrgang
416c215018 change default order of posts (#7096)
closes #6932
- new default order of posts: scheduled, draft, published
- invent orderDefaultRaw fn for each model
- each model is able to create a default raw order query
- separate count and fetch query for fetchPage, because the count query where group/order statements attached
2016-07-15 11:04:10 +01:00
Katharina Irrgang
f644d99460 add small permission improvements
no issue
- do not check client type in auth middleware
- offer filtering for findAll function in base
- add isInternalContext to base model
2016-05-08 14:22:55 +02:00
rfpe
7abcc43907 Harvest server side strings
closes #5617
- Replace all hard-coded server-side strings with i18n translations
2015-12-19 12:12:16 +01:00
Hannah Wolfe
14c3bd605e Ensure filter parse errors generate 422 not 500
no issue

- This resolves some issues we've seen with Ghost throwing 500 errors when the filter parameter is incorrectly structured/formatted
2015-12-15 15:01:06 +00:00
Hannah Wolfe
d4c8b69673 Support ordering by count
refs #6009

- super quick and dirty way to support ordering by counts
- @TODO refactor :)
2015-11-22 17:18:30 +00:00
Hannah Wolfe
2aa16514a3 Rename post_count to count.posts
refs  #6009

- This is a straight rename, no functionality is added
- The dot syntax requires pre/post processing to convert the name
- This PR also includes several updates to the tests, as they weren't being run as part of Travis!
2015-11-20 14:59:58 +00:00
Sebastian Gierlinger
55564e3daf Merge pull request #6091 from ErisDS/safe-count
Safe post_count for Tags & Users
2015-11-19 10:34:14 +01:00
Hannah Wolfe
770f45245c Safe post_count for Tags & Users
refs #6009, #5614

- Use the new isPublicContext method to detect whether to add extra clauses to the count
- Add count to users
2015-11-18 19:19:11 +00:00
Hannah Wolfe
f8d9af1010 Add some debugging tools to filters
- pass debug: true to the API to get some useful debug output
- does not work in production mode

Note: I have added these lines back in so many times in the past month or so so that I could
figure out what was happening, I figured everyone else might find them useful.

TODO: use a proper logging method dependent on env
2015-11-18 19:17:06 +00:00
Hannah Wolfe
6a0f1cf231 Filter plugin with enforce/default logic
refs #5614, #5943

- adds a new 'filter' bookshelf plugin which extends the model
- the filter plugin provides handling for merging/combining various filters (enforced, defaults and custom/user-provided)
- the filter plugin also handles the calls to gql
- post processing is also moved to the plugin, to be further refactored/removed in future
- adds tests showing how filter could be abused prior to this commit
2015-11-17 10:39:44 +00:00
Hannah Wolfe
666a616551 Add access rules bookshelf plugin
refs #5614

- change isPublicContext to detectPublicContext
  - behaviour now expands the context object out
  - this is a bit of a sideeffect, but this is the simplest change
    that makes it possible to use the context in the model layer without
    significant wider changes
- add new access rules plugin
  - takes a context object as part of `forge()` & caches it on the model instance
  - provides helper functions for testing access rules later on
2015-11-16 12:24:01 +00:00
Hannah Wolfe
8edf382b40 Move bookshelf plugins into own folder 2015-11-11 19:40:12 +00:00