A resource can now be tested like this:
```ruby
RSpec.describe DiscourseDataExplorer::QueryResource, type: :resource do
it { is_expected.to declare_type(:queries) }
it { is_expected.to expose(:sql).readable_by(admin.guardian).hidden_from(Guardian.new) }
it { is_expected.to have_one(:user) }
it { is_expected.to sort_on(:name, :last_run_at, "user.username") }
it { is_expected.to filter_on(:search) }
it { is_expected.to paginate(default: 50, max: 100) }
end
```
It’s heavily inspired by shoulda-matchers.
The queries endpoint now answers JSON:API at
`/api/data-explorer/queries`. The old endpoint stays for now.
A plugin declares an endpoint in two small files: a resource that says
what the endpoint offers, and a controller that names it.
Two things change for a caller. The author and the groups are
relationships now rather than plain fields, which follows the JSON:API
spec, and paging is by cursor rather than by offset, which is the point
of the profile we apply.
The total row count and the queries that ship with the plugin without
being rows are not covered yet.
A resource used to show every field it declared to everyone, so there
was no way to serve something like `sql` to admins only.
A field can now declare it:
```ruby
attribute :sql, readable: ->(guardian) { guardian.is_admin? }
attribute :notes, readable: ->(guardian, record) { guardian.can_edit?(record) }
```
When the rule only looks at the user, the answer is the same for the
whole request, so the column is never selected. When it also looks at
the record, the column has to be read and the decision happens row by
row.
A field the user can’t see is left out, with no error (and requesting it
by name does not change that).
Relationships take the same option, but only in the user form. A rule
that looks at a record raises when the resource loads: there, "the
record" could mean the owner row or each related row, and a related
resource already scopes its own.
The JSON:API Kit pages by cursor and that’s a JSON:API profile.
The specification requires the profile in the `Content-Type` header. It
also recommends it in the top-level `self` link. Every response now
carries both.
This PR wires the Kit’s document generation into a base controller. It
inherits `ApplicationController`, so an endpoint keeps authentication,
API keys and rate limiting.
It also negotiates as the specification requires. It answers 415 for a
media type parameter we do not support. It answers 406 when an `Accept`
header allows nothing we send. Every response carries the JSON:API media
type.
The default format is JSON: an unexpected error becomes a JSON:API error
document. An error Discourse’s core handles will be served as JSON
instead of an HTML page.
A client sends `sort=-created_at` but `Resource` takes `sort: {
created_at: :desc }`. The contract already validates everything, so it’s
logical for it to also convert values. That’s necessary for wiring the
kit in a controller.
A caller that already passes the kit's own parameters gets them back
unchanged, so nothing below the contract had to move.
Discourse's API grew one endpoint at a time, and each endpoint response
is a custom one. The JSON:API Kit relies on the JSON:API 1.1
specification, so requests and responses are standardized.
This commit adds the core framework and nothing else. No route points at
it and no existing endpoint changes.
Integration specs live in `spec/integration/json_api_kit` and cover the
various JSON:API concepts. Each example assert a rendered document. This
is a good entry point for reviewers.
```ruby
class TopicResource < JsonApiKit::Resource
model Topic
type :topics
attribute :title
attribute :created_at
has_one :user, resource: UserResource
has_many :posts, resource: PostResource
includes "user.groups"
sort :created_at
sort :title
default_sort created_at: :desc
filter :title
anchor :id
page default: 20, max: 100
scope { |guardian| Topic.secured(guardian) }
end
```
## What works
- Documents: collections and a single record both render `data`,
`included` and `links`. Every resource object carries its `type`, `id`,
`attributes`, `relationships`, a `self` link, and its own cursor under
`meta`.
- Sparse fieldsets: a client asks for the fields it wants per type, and
the query returns those columns only.
- Related resources: a client provides a path, and the document returns
every record of that path once (no duplicate).
- Sorting: a client provides a sort the resource declares, with a
direction.
- Filtering: a client provides a filter the resource declares, with a
value or a list of values.
- Pagination: every page is read from a cursor, which follows the cursor
pagination profile at
https://jsonapi.org/profiles/ethanresnick/cursor-pagination. A
collection links to `prev` and `next`, and every row carries the cursor
that reads the page from it. The kit adds one extension to that profile:
a page centred on a row
(called an anchor).
- Authorization: a resource declares its scope, and the scope reads the
guardian. A record the scope hides answers 404, and a related record it
hides renders as an empty relationship.
- Errors: a bad request renders `errors`, and each error carries a
`status`, a `title`, a `detail` a person can act on, and the
`source.parameter` that caused it.
## What’s not working yet
- Controller/endpoint: nothing is wired yet, so it can’t serve a real
request from an endpoint.
- Fields: no restricted fields, type, description or example yet.
## What’s next
The first real endpoint, which turns these parts into an end-to-end
path.
Three independent issues in `each`'s isolation, each surfacing as soon
as the step is used outside the simplest shapes:
1. Nested `each` blocks crashed inside `with_isolation`'s `ensure`. The
isolation kept its snapshot in a single slot that the inner call would
overwrite then null, so the outer's cleanup ran against `nil`. The
snapshot now lives on a stack, making isolation re-entrant.
2. ActiveRecord models in the context lost their primary key inside an
`each` block. The snapshot used `deep_dup`, which recurses into AR
objects via `dup` and AR's `dup` returns an unpersisted copy with `id ==
nil`. Basic shapes like `model :user; each :things do ... end` silently
swapped the real user for a useless ghost. The deep copy was originally
an attempt at mutation isolation for collections, but that contract was
never documented, its supporting spec was removed before the original PR
merged, and the rest of the framework already lets steps mutate what
they receive. A shallow dup matches the documented "variables set inside
the loop don't leak" guarantee and stops corrupting models.
3. Non-persisted state leaked between iterations. The whole loop ran
inside one isolation, so iteration N could read scratch values iteration
N-1 had left around. Each iteration now gets its own isolation. Steps
inside the same iteration still share state freely (step 2 can act on
what step 1 produced); only cross-iteration carry-over now requires
`persist:`, which makes that dependency visible at the `each`
declaration.
Asking `UpcomingChanges.enabled?` about a key that isn't registered used
to crash deep inside `meets_or_exceeds_status?` with a confusing
`NoMethodError (undefined method '>=' for nil)`. That made stale callers
(e.g. controllers still gating on a key whose `upcoming_change` metadata
has been removed) hard to trace back from a 500.
Raise an `ArgumentError` at the public entry point instead, so the bug
surfaces with a clear "Unknown upcoming change: X" message and points
straight at the offending caller.
The `enable_form_templates` site setting was recently demoted from an
upcoming change to a regular site setting, but the controllers were
still gating access through `UpcomingChanges.enabled_for_user?`. Without
the upcoming change metadata, that path crashed with `NoMethodError
(undefined method '>=' for nil)` whenever the setting sat at its default
value, taking the admin Form Templates page down with it.
Switch the two controllers to a plain
`SiteSetting.enable_form_templates` check, and drop the redundant
`before` hooks from the request specs so they actually exercise the
default-value path that broke in production.
Services that process collections (bulk delete, bulk create, etc.)
currently require manual iteration inside a `step`, losing the
framework's built-in error handling and inspection. This adds a
first-class `each` step that brings the full service DSL inside the
iteration loop.
```ruby
each :users do
policy :can_delete
step :destroy
end
```
Each iteration receives the singularized item name (`user:`) and
`index:` as keyword arguments. If any nested step fails, iteration stops
and the failing item/index remain in context for error reporting.
Existing matchers (`fail_a_policy`, `fail_a_step`, etc.) work inside
each blocks.
A `persist:` option allows values to accumulate across iterations and
survive the loop's variable isolation:
```ruby
each :tag_names, persist: { results: -> { { created: [], failed: [] } } } do
step :create_tag
end
# context[:results] available after the loop
```
The steps inspector displays iteration progress ((3/3) on success, (1/3)
when failing at the first item, (empty collection) with ⏭️ when
skipped).
Pitchfork has been the default web server for some time now. This
removes Unicorn entirely to simplify the codebase and unblock future
improvements (like Rack 3).
Notable changes beyond the straightforward removal:
- `Discourse.after_unicorn_worker_fork` →
`Discourse.apply_worker_db_variables_overrides`: renamed and wired into
pitchfork.conf.rb's `after_worker_fork`. This actually *fixes*
per-worker DB variable overrides (`unicorn_worker_db_variables_*`) which
were never called under Pitchfork.
- `bin/ember-cli`: `--unicorn` flag renamed to `--server` (`-u` kept).
- `lib/demon/sidekiq.rb`: removed Unicorn-specific USR1/USR2 signal
handlers and `reopen_logs` (called `Unicorn::Util.reopen_logs`), which
were already dead code under Pitchfork.
Intentionally kept unchanged:
- `config/unicorn_launcher` (used by Docker images, separate effort)
- `docker_manager` plugin (separate repo)
- `UNICORN_*` env vars (renaming deferred)
- Rack < 3 constraint (separate PR)
When pressing Ctrl+C with `bin/ember-cli -u`, the server could appear
stuck for up to 60 seconds and stop responding to further Ctrl+C
attempts, requiring `kill -9` to terminate.
This was most commonly triggered after switching git branches, which
could leave workers stuck in native code (autoloading, V8 compilation,
etc.), making the slow shutdown path much more noticeable.
The root cause was a signal race: when Ctrl+C sends SIGINT to the
process group, ember-cli's monitoring thread would also send SIGTERM to
the pitchfork supervisor after detecting ember died. This TERM arrived
at the pitchfork monitor while it was already performing a hard shutdown
(from INT), causing it to abort the hard shutdown and fall back to a
slow graceful shutdown with a 60-second timeout. Combined with both
bin/ember-cli and bin/pitchfork unconditionally swallowing all
subsequent SIGINTs, the user had no way to interrupt.
Fixes:
- bin/ember-cli: track whether SIGINT was received and skip sending TERM
to the server in that case (it already got INT from the terminal)
- bin/ember-cli, bin/pitchfork, bin/unicorn: add signal escalation so
repeated Ctrl+C can force-kill children as a last resort
During recent refactoring, we lost the ability to bump versions on
release branches. Rather than restoring the separate workflow and rake
task that existed before, this integrates version bumping directly into
the existing release automation:
1. When `release:maybe_cut_branch` creates a new release branch, it now
strips the `-latest` suffix and commits the release version directly
(e.g., `2025.1.0-latest` → `2025.1.0`).
2. When `release:stage_security_fixes` targets a release branch, it now
bumps the patch version automatically (e.g., `2025.6.0` → `2025.6.1`).
For non-security patch releases, the existing workflow handles tagging
automatically when commits are pushed to release branches with a
manually bumped version.
The `/raw/:topic_id` endpoint was exposing hidden posts to users who
shouldn't see them, while `/raw/:topic_id/:post_number` correctly
filtered them via `guardian.can_see?`.
This inconsistency meant anonymous users and regular users could see
hidden post content when fetching a whole topic as markdown.
Also refactors `markdown_num` into smaller private methods for clarity.
When security fixes are staged for the main branch via the
`stage_security_fixes` task, the development version is now
automatically incremented (e.g. `2026.2.0-latest` to
`2026.2.0-latest.1`). This ensures each security fix batch gets its own
tagged version, allowing docker_manager to detect that installations on
`latest` need a critical update.
To support this, a new `ReleaseUtils::Version` value object encapsulates
version parsing, comparison, and manipulation logic that was previously
done through ad-hoc string splitting and `Gem::Version` comparisons in
release.rake. It understands Discourse's versioning scheme
(major.minor.patch-pre.revision) and provides methods like
`#next_revision`, `#same_development_cycle?`, and
`#next_development_cycle`.
All existing rake tasks have been refactored to use `Version` objects,
the unused `parse_current_version` helper and
`prepare_next_version_branch` task have been removed, and the release
specs have been rewritten following RSpec style guide conventions.
Recently, the `lock` step from our Ruby service framework has been
extended to be able to work with keys from the global context when the
key isn’t present in the `params`.
The implementation is flawed because we usually get a contract object in
the `params` key of the context, and currently we call
`public_send(key)` on it. This actually raises an error when the key
doesn’t exist on the contract.
The fix is rather simple, we just use `try` instead, that way if the
method doesn’t exist, it returns `nil`, allowing us to get the key from
the global context as expected.
The `base.rb` file (640+ lines) contained 9 step classes, the `Context`
class, and the `StepsHelpers` DSL module alongside the core concern.
Each is now in its own file under `lib/service/base/`, following the
existing `lib/service.rb` + `lib/service/*.rb` pattern.
Also:
- Update stale inline documentation in `lib/service.rb`
- Use `NotImplementedError` instead of generic raise in `PolicyBase` and
`ActionBase`
- Move YARD DSL docs to a `@!parse` block at the top of `base.rb`
Previously, the `lock` step could only build its lock name from values
present in `params`. This made it impossible to lock on values derived
from earlier steps, such as a model fetched from the database.
The lock name resolution now falls back to the service context when a
key isn't found in `params`. When the resolved value is a model
(responds to `id`), its id is used instead of the object itself,
producing a meaningful lock name.
This is fully backward compatible: existing callers that pass param keys
still resolve from `params` first.
Pitchfork defaults `setpgid` to `true`, which calls
`Process.setpgid(pid, pid)` on each forked worker, moving it into its
own process group. This detaches the worker from the terminal's
foreground process group, so any attempt to read STDIN (e.g.
`binding.pry`) fails with:
Error: Input/output error @ io_getpartial - <STDIN>
Unicorn never called `setpgid`, which is why `binding.pry` worked there.
Setting `setpgid false` in non-production environments restores that
behavior and allows interactive debuggers to function.
The monolithic `CategoryHierarchicalSearch` service mixed query
building, eager loading, and pagination logic in a single class. The
query was a large raw SQL string built through conditional string
concatenation. Fragments like `#{matches_sql}`, `#{only_ids_sql}`,
`#{except_ids_sql}` were stitched together, with LIMIT/OFFSET appended
via ternary interpolation and named placeholders passed through a
manually assembled hash. This made the query fragile and hard to follow.
Break it into focused, single-responsibility classes under the
`Category::` namespace:
- `Category::HierarchicalSearch` — service orchestrator using
`Service::Base`, with a contract that owns pagination logic (page
validation, limit/offset computation)
- `Category::Query::HierarchicalSearch` — query object that uses
ActiveRecord's interface where it naturally fits (`.where()` with
parameter binding, `.limit()`, `.offset()`, `.with()`, `.joins()`) and
isolates the genuinely complex SQL (recursive CTEs, term matching) into
small named methods rather than a monolithic heredoc
- `Category::Action::EagerLoadAssociations` — extracted eager loading
into a reusable `Service::ActionBase`
The controller is simplified to a single
`Category::HierarchicalSearch.call(service_params)` call with proper
`on_success` / `on_failed_contract` / `on_failure` handling, replacing
manual param transformation and direct result access.
Specs are rewritten to test each class in isolation: the service spec
stubs its collaborators to verify orchestration, the query spec
exercises actual SQL behavior, and the action spec verifies preloading.
Service structure and spec patterns follow the [Discourse service object
guidelines](https://meta.discourse.org/t/using-service-objects-in-discourse/333641)
and the [RSpec Style Guide](https://rspec.rubystyle.guide/).
On constrained hardware/environments, the splay we added for Pitchfork’s
workers isn’t always enough and they hit the spawn timeout which is 10
seconds by default.
This changes the default to 60 seconds, which should give plenty of time
for booting even in the worst conditions. Unicorn didn’t have this
timeout, explaining why we never saw this phenomenon before.
The `APP_SERVER_SPAWN_TIMEOUT` env var can be used to lower or increase
the timeout value.
This changes the logic from opt-in to opt-out regarding Pitchfork.
Now, instead of having to explicitly set `RUN_PITCHFORK` to `1`, it has
to be set to `0` to switch back to Unicorn.
This is a follow-up to fd04f690.
The original commit only protected potentially illegal reviewables from
being auto-approved in one code path (direct post deletion). However,
when deleting a post through a reviewable action with
`notify_users_after_responses_deleted_on_flagged_post` enabled, the code
would still call `#ignore` on any flagged reply posts without checking
`#potentially_illegal?`.
This is a follow-up to d50ec29f92.
Here, we’re applying what’s been done for Unicorn, as it shows better
results than setting `spawn_timeout` to a higher value. The overall boot
will be a bit slower, but it will put less pressure on the system and
will work better in the end.
When deploying many workers, their startup can be relatively slow
because of the warm up we’re doing in the `after_work` hook.
The default time out when spawning a worker is 10 seconds, which is
usually fine, but sometimes more time is needed.
This patch sets `spawn_timeout` to the same value as `timeout`.
When selecting various posts from the search UI to be deleted using the
bulk actions, an error can be encountered saying, "You are not permitted
to view the requested resource". That’s because it doesn’t work when the
post is the first post of a topic.
This patch addresses the issue by checking if the user can either
destroy the post or the related topic. The logic for destroying a topic
is the same as the one for destroying a post since in both cases we’re
calling `PostDestroyer#destroy`.
Sometimes, HTML entities can be escaped twice, typically when getting
sanitized data from our `Onebox::OpenGraph` class then providing that
value to a template. We’re using the Mustache gem to process the Onebox
templates, and it will automatically escape HTML entities. This is
usually not a problem, but it is for things like ampersands. For
example, if the value we provide to the template is `&`, then
Mustache will convert it to `&amp;`.
This patch fixes that behavior by decoding the result of the sanitization
we apply in `Onebox::OpenGraph`. That way, templates will get `&`
instead of `&`, thus there won’t be any double escaping.
Currently, when a flagged post is deleted directly from the topic, the
reviewable is automatically approved. While this seems sensible in most
cases, when the post is flagged as illegal, it can become a hassle for
some forum operators (that have to report things because of the DSA, for
example) when the post wasn’t really illegal.
This brings the necessary changes to allow the `docker_manager` plugin
to work properly with Pitchfork (see
https://github.com/discourse/docker_manager/pull/293).
Three things are needed:
- Tell our MessageBus config to not return a 429 if a request takes more
than 100 ms while the Pitchfork server is restarting.
- Update the Pitchfork config to allow it to reuse its own port (this
allows us to start a new Pitchfork server while the old one is still
running)
- Update the `unicorn_launcher` script to restart the Pitchfork server
when it receives a USR2/HUP signal. This way, it doesn’t complicate
things too much.
This change adds the ability to validate more complex structures in the
Ruby service contracts.
Contracts were limited to flat structures, which is fine most of the
time, but it can become tedious when managing lots of attributes.
With this new feature, contracts like this one can be defined:
```ruby
attribute :channel_id, :integer
attribute :record, :hash do
attribute :id, :integer
attribute :created_at, :datetime
attribute :enabled, :boolean
end
attribute :user, :hash do
attribute :username, :string
attribute :age, :integer
validates :username, presence: true
end
attribute :items, :array do
attribute :name, :string
validates :name, presence: true
end
validates :channel_id, presence: true
```
Two nested types are available: `hash` and `array`.
Each block creates a new contract, meaning coercions, validations and
callbacks are available as usual.
`ActiveModel` doesn’t have a `symbol` type by default, but it can be
quite handy for contracts from our service framework.
This way, it’s easy to get a symbol from an attribute. Any non-blank
value will get converted to a symbol by being converted to a string
before. A blank value will return `nil`.
Currently, when uploads have a different SHA1 in their URL from the one
that has been computed (it can happen with secure uploads), and those
uploads are images, then the cooked post processor won’t link them
properly to their post. The post has them as `Post#uploads` but not as
`Post#image_upload`. This in turn will make the associated topic
thumbnail-less when the post is the first one.
To address the issue, it’s a matter of adding fallbacks to
`CookedPostProcessor#update_post_image` when fetching the uploads. This
is something we already do in
`HasPostUploadReferences#link_post_uploads`.
Currently, when a Pitchfork worker times out, we’re logging its
backtrace as a warning. After that, Pitchfork will terminate cleanly the
worker by calling `Process.exit`. This, however, raises a `SystemExit`
exception that is caught by Logster. Since we already have the backtrace
logged, we don’t need to log that exception which is more confusing than
anything else.
This change makes Logster ignore that `SystemExit` exception and raises
the log level to error instead of warning.
When a user is invited to an existing private message, they were not
receiving notifications for subsequent messages. This occurred because
`TopicUser` records are created lazily when a user first visits a topic,
but `PostAlerter#notify_pm_users` checks the notification level
immediately when determining who should receive notifications.
This PR explicitly creates a `TopicUser` record with its notification
level set to `watching` immediately after granting PM access, ensuring
the invited user receives notifications for all subsequent messages even
before their first visit.
We don’t want to dump the DB schema as it can lead to various problems,
mainly with plugins.
We already have some patches that take care of that, but sometimes a
`db/structure.sql` file is still generated.
The solution is to instruct Rails to never dump that schema, even in the
test and development environments.
Currently, a thumbnail is usually generated when generating oneboxes for
Youtube videos. This happens because of the lazy-videos plugins. But
when it’s not enabled or it can’t fetch the metadata as expected, we
fallback on the original Youtube onebox.
That onebox just outputs an iframe and nothing more. This means no
thumbnails will be created for the associated topic.
This patch addresses the issue by outputting an invisible image before the
iframe, thus allowing thumbnails to be created. This makes the user
experience more consistent, whether using the classic onebox or the
lazy-videos one.
This change aims to allow contract reusability. Typically when two
services are closely related (like `create`/`update`), a lot of (maybe
even all of them) parameters could be the same.
So instead of having to redefine virtually the same contract twice, with
this change it’s possible to provide a base class for the contract, like
this:
```ruby
params base_class: MyCreationService::Contract
```
Currently, when a plugin is disabled, its various callbacks are still
taken into account. This can lead to performance issues.
This patch adds a check to most of the `register_*` methods a plugin can
use, so if the plugin is disabled, those callbacks won’t be applied.
This PR adds Pitchfork, as we want to move away from Unicorn ultimately.
Unicorn still boots by default, so there should be no disruption for
anyone.
To use Pitchfork instead of Unicorn, the `RUN_PITCHFORK` environment
variable must be set.
This will make `bin/rails s` and `config/unicorn_launcher` boot
Pitchfork. `unicorn_launcher` was patched because that way we can easily
switch between Unicorn and Pitchfork without having to change too many
things on the infra side.
The upgrader from the `docker_manager` plugin doesn’t work yet with
Pitchfork. This will be addressed in a future PR.
Instead of having to clean an array in a contract using a
`before_validation` block, for example, we can now pass `compact_blank:
true` to the attribute, like this:
```ruby
attribute :ids, :array, compact_blank: true
```
When precompiled assets exist, Propshaft will switch to a static mode
and only serve those existing assets. This confuses people on a regular
basis, so this patch addresses that issue by displaying a banner on
STDOUT if precompiled assets exist in a local env (test or dev).
This patch introduces a new step to the Ruby Service Framework.
`only_if` will execute its block (other steps) only if the provided
condition evaluates to `true`. As for the other steps, the name provided
is the name of the method that will be executed.
```ruby
only_if(:can_update) do
model :user
step :update_user
step :notify
end
private
def can_update(guardian:)
…
end
```
This step cannot fail and the whole block will be skipped if the method
evaluates to a falsy value.
This is rendered by the steps inspector:
```
[ 1/13] [options] default ✅
[ 2/13] [model] model ✅
[ 3/13] [policy] policy ✅
[ 4/13] [params] default ✅
[ 5/13] [lock] parameter:other_param ✅
[ 6/13] [transaction]
[ 7/13] [step] in_transaction_step_1 ✅
[ 8/13] [step] in_transaction_step_2 ✅
[ 9/13] [try]
[10/13] [step] might_raise ✅
[11/13] [only_if] condition ⏭️ (condition was not met)
[12/13] [step] optional_step
[13/13] [step] final_step ✅
```
The inspector also renders text with colors now.
<img width="419" height="336" alt="Copie d'écran_20251008_162920"
src="https://github.com/user-attachments/assets/add4fd16-076e-4f30-ae1a-3e933494967e"
/>
This PR uses MessagePack instead of JSON for serializing our cookies.
MessagePack is almost as fast as Marshal but without the security
issues. It’s also able to serialize more objects than JSON (like Time,
Symbol, etc.). As it’s a binary format, it takes less space than JSON,
sometimes half less. Finally, MessagePack isn’t Ruby-specific and
implementations exist in every existing language.
Regarding the cookies Discourse is using, we can see a small improvement
on the `_forum_session` one when it’s almost empty (around 2%), but the
more things are put into it, the more we’ll see savings. For the `_t`
cookie, we’re saving around 20% for free.
Now that `ServerSession` can store arbitrary data, we can move some more
data into it.
This patch moves some data related to authentication into it, as
sometimes that kind of data can be pretty big.
Reverts discourse/discourse#34639
We understood the main culprit for cookie overflows was storing
`destination_url` in the session, so we don’t really need that debug
code anymore.
Now that `ServerSession` can store arbitrary data, we can move some more
data into it.
This PR moves some data related to authentication into it, as sometimes
that kind of data can be pretty big.
Currently, the server session can only store strings. As we want to move
more things into it (like with a proper session object), we need to be
able to store arbitrary data.
This PR serializes data using Message Pack, as it’s almost as flexible
as Marshal but without the potential security issues.
Recently we replaced the `pry-byebug` gem by the `debug` one. It broke
the auto-reload mechanism we have in the development environment for
Unicorn.
Indeed, the `debug` gem by default will add an `at_exit` hook that will
wait for all its children to exit. It clashes with our own mechanism.
The workaround is to require `debug/prelude` instead of `debug`: the
`debugger` command and breakpoints will still work but without the hook
being set.
Since the session is backed by a cookie, storing too much data will lead
to a cookie overflow error.
A return path can be quite large sometimes, so intead of storing it in
the session, this patch stores it in our server session.
This patch will be followed by
https://github.com/discourse/discourse/pull/34747.
`SecureSession` doesn’t make a lot of sense anymore and can be confusing
as the current cookie store used for the session is actually secure
since it’s encrypted.
Renaming it to `ServerSession` better conveys what it does: providing a
session but on the server side only.
This patch also makes some improvements, like injecting that server
session into Rack-like request objects, allowing the server session to
be available virtually everywhere.
This patch logs what’s in the cookie when there is an overflow, as it
happens sometimes during auth workflows. This should help us better
understand what’s happening.
We recently updated `Chat::ListChannelThreadMessages` to take an option
so its `max_page_size` could be configured.
This behavior should be consistent between
`Chat::ListChannelThreadMessages` and `Chat::ListChannelMessages` since
they’re basically doing the same thing.
This patch updates the behavior of the `Chat::ListChannelMessages`
service.
- Introduce a `max_page_size` option, allowing different behavior
between controllers and SDK.
- Improve the contract (validations & helper method).
- Use `model` where possible.
- Extract message existence logic to a dedicated policy, allowing easier
testing.
- Refactor specs to follow current guidelines/best practices.
Currently, it can happen that the `Chat::AutoJoinChannels` service
raises a `PG::UniqueViolation` error. This is probably due to a race
condition. That exception is not rescued, leading to 500s.
This PR wraps the main step inside a `try` block and also inside a
`lock` block.
- Migrated from annotate to annotaterb as the former is not maintained
anymore.
- Dropped our `fast_pluck` patch as the default `pluck` implementation
seems now faster.
Currently, there’s an edge case in the calendar plugin: when a recurring
event (typically a daily one) ends on the next day, it can happen that we
create a `event_date` record with no `starts_at` attribute.
This is because `Event#calculate_next_date` doesn’t check the result
from `RRuleGenerator.generate`.
This patch addresses the issue simply by checking the value of
`RRuleGenerator.generate` and returns early if the value is `nil`.
Currently, `#event_starts_at` and `#event_ends_at` are not properly
serialized: we just output the string we have in the custom fields,
which is not necessarily a time string JS can deserialize. Until now it
worked by chance.
With Rails 8, the string format changes a bit to include the timezone,
breaking the deserializing process on the JS side.
This patch addresses the issue by converting the value stored in the
custom fields to a proper time object. Then the serializers, when
rendering JSON, will output a proper ISO8601 formatted string.
- improve the contract a little
- use `model` where possible
- extract message existence logic to a dedicated policy, allowing easier
testing.
- remove unused code
- refactor specs to follow current guidelines/best practices
When fetching a model using the `model` step in a service, if that model
is an `ActiveRecord` object, we check if it’s in a valid state. While
this is useful when manipulating the model or when we create a new one,
it’s not the case for a model we just pulled from the DB, as it should
be valid.
In some cases, running the validations can be costly (it can lead to N+1
queries if the model validates associated items for example).
This patch introduces a small optimization by checking if the model has
any pending changes on it, thus requiring validation. If that’s not the
case, we just skip the validation part, as the model should be valid
anyway.
Currently when a model is not found, we raise an `ArgumentError`
exception and that exception is stored in the resulting context object.
However, since we’re also storing unexpected exceptions, this default
exception can pollute the context object when we need to inspect it or
act on it.
This patch addresses that issue by raising a custom exception instead,
and we then discard it from the context object.
- Move some data transforming into contracts.
- Add some missing specs.
- Use the `try` step.
- Improve the `model` step a bit by allowing to catch any exception,
and not only `ArgumentError`. We already had the mechanism to inspect
which exception was caught.
This patch adds a new shared example to be used as a smoke test in
plugins and themes.
A `skip_examples` argument is available to easily opt-out from a
category of tests.
Example:
```rb
RSpec.describe "Testing core features", type: :system do
it_behaves_like "having working core features", skip_examples: %i[search login]
end
```
As we’re currently using a namespace for Sidekiq, in order to upgrade to
the latest version, we need to drop it as it’s not supported anymore.
The recommended way is to use a different Redis DB for Sidekiq.
This patch uses a different config for Sidekiq and also takes care of
migrating existing jobs (in queues and the retry and scheduled sets).
A previous refactor of the `Service::Base::Step` class introduced a
non thread-safe behavior. `#call` mutates instance variables at runtime,
and since a step instance is the same for any given service class, this
can sometimes lead to `context` being the wrong one for the running
service.
This patch makes use of `Concurrent::ThreadLocalVar` to fix the issue.
This patch adds two things:
1. An outcome matcher (`on_lock_not_acquired`), allowing to react when
there was a problem with the lock.
2. Compatibility with the steps inspector, allowing to display properly
the steps of a service containing locks.
There’s currently a bug when using a dedicated class as a policy in
services: if that class delegates its `#call` method (to an underlying
strategy object for example), then an error will be raised saying steps
aren’t allowed to provide default parameters.
This should not happen, and this patch fixes that issue.
Currently, there are two ways (kind of) for accessing `params` inside a
service:
- when there is no contract or it hasn’t been reached yet, `params` is
just the hash that was provided to the service. To access a key, you
have to use the bracket notation `params[:my_key]`.
- when there is a contract and it has been executed successfully,
`params` now references the contract and the attributes are accessible
using methods (`params.my_key`).
This patch unifies how `params` exposes its attributes. Now, even if
there is no contract at all in a service, `params` will expose its
attributes through methods, that way things are more consistent.
This patch also makes sure there is always a `params` object available
even when no `params` key is provided to the service (this allows a
contract to fail because its attributes are blank instead of having the
service raising an error because it doesn’t find `params` in its context).
This patch aims to improve the steps inspector output:
- The service class name is displayed at the top.
- Next to each step is displayed the time it took to run said step.
- Steps that didn’t run are hidden.
- `#inspect` automatically outputs the error when it is present.
Loïc Guitaut