fix: emit warning for legacy target names by steve-chavez · Pull Request #4905 · PostgREST/postgrest

steve-chavez · 2026-05-09T02:21:31Z

Reverts #4104 and eases transition towards the breaking change.

When the request with the deprecated syntax comes we log a warning, like so:

$ PGRST_LOG_LEVEL=info postgrest-with-pg-14 -f test/spec/fixtures/load.sql postgrest-run

$ curl 'localhost:3000/projects?id=eq.1&select=id,name,the_tasks:tasks(id,name),clientes:clients(*)&tasks.order=name.asc&clients.id=eq.1'

08/May/2026:21:10:24 -0500: WARNING: Embedded resource was referenced by relation name even though it has an alias. This is deprecated and will stop working in a future release.
08/May/2026:21:10:24 -0500: Please update the filters that use `tasks` to `the_tasks`, `clients` to `clientes` in `GET /projects?id=eq.1&select=id,name,the_tasks:tasks(id,name),clientes:client
s(*)&tasks.order=name.asc&clients.id=eq.1`
127.0.0.1 - postgrest_test_anonymous [08/May/2026:21:10:24 -0500] "GET /projects?id=eq.1&select=id,name,the_tasks:tasks(id,name),clientes:clients(*)&tasks.order=name.asc&clients.id=eq.1 HTTP/1
.1" 200 146 "" "curl/7.81.0"

Note that:

The request is repeated on the WARNING message. This is because the error logs go to stderr and access/apache logs to stdout, we can't ensure they appear at the same time.
Includes all the alias that need to be corrected "tasks to the_tasks, clients to clientes"

TODO

Add a config that allows user to opt in to this breaking change and removes the warning. Let's have a single config for all cases that can come up in the future. Like server-legacy-features=target-names.
Tests. fix: unexpected results when embedding the same table twice #4104 didn't include a test that confirms old target names didn't work.
Loadtests. Add a test in test/load/targets.http that uses the alias to see the impact of this new logic. This should be done in another PR.
~~The warning only happens for log-level=warn, this requires Switch to log-level=warn as default #4904 to be effective.~~ Not planned anymore, see also: fix: emit warning for legacy target names #4905 (comment)

steve-chavez · 2026-05-09T02:33:40Z

+  , relSpread                  :: Maybe SpreadType
+  , relSelect                  :: [RelSelectField]
+  , depth                      :: Depth -- ^ used for aliasing
+  , relIsLegacyTargetNameMatch :: Bool  -- ^ used to ease migration into a new version with breaking change


Main idea is to add an attribute to ReadPlan, that is then aggregated in App.hs with legacyWarnings to generate the logs. That keeps the Plan module pure.

laurenceisla · 2026-05-10T20:42:31Z

Includes all the alias that need to be corrected "tasks to the_tasks, clients to clientes"

While working on building this warning, I noticed that we might avoid the breaking change if we look for the filter twice in the list of embeddings:

We check if the relation alias is present in the embeddings list and use the filter on that embedding if found
If not, then we make the search in the list again, but we look for relation names now

All tests pass when I try this, including the one that revealed the breaking change, but there could be outliers that may still show a breaking change so I'd have to look for them. I think it's worth the try, but before doing that @steve-chavez do you see it as a viable alternative against this whole breaking change warning stuff? (this would still be useful for future breaking changes though)

steve-chavez · 2026-05-10T20:56:30Z

@laurenceisla I'd say go ahead if you think it's doable without a breaking change (not sure if possible). But we do need to add loadtests for these alias cases to see if the double searching you propose won't cause a noticeable regression.

wolfgangwalther · 2026-05-11T04:11:34Z

I think we should do the breaking change at some point anyway. The previous behavior was just wrong - once you rename it via an alias, that's the name it should go by. If you want another release with a warning etc. I'd be OK with that. But I'd also be OK without that added complexity and the status quo (the breaking fix as it is currently done on main).

laurenceisla · 2026-05-11T14:36:35Z

(not sure if possible)
The previous behavior was just wrong

Right, after pondering it for some time, I believe the breaking change is inevitable. Will continue with the TODO list for this PR.

steve-chavez · 2026-05-20T21:38:04Z

Now that #4904 is closed. Can we still emit the WARNING: logs I mentioend above? I'd think so given it's critical for the administrator to somehow communicate this to clients.

@wolfgangwalther WDYT?

steve-chavez · 2026-05-21T19:34:40Z

@laurenceisla How about merging the first revert in another PR? I think we agree that it's not right to make the breaking change this way. Otherwise we risk this going into a new release somehow.

wolfgangwalther · 2026-05-21T19:39:03Z

Can we still emit the WARNING: logs I mentioend above? I'd think so given it's critical for the administrator to somehow communicate this to clients.

@wolfgangwalther WDYT?

As I wrote in an earlier comment, I am not opposed to that. I don't feel like it's necessary, but if you do, I'm ok with it.

I think we should not revert it - because we will need to do it anyway, for correctness. So we better deal with it in a way that makes you feel good about it.

laurenceisla

This should be ready for review now.

Loadtests. Add a test in test/load/targets.http that uses the alias to see the impact of this new logic. This should be done in another PR.

Do you mean to test how it's currently working on main and to check the impact of this PR?

wolfgangwalther · 2026-06-02T06:44:30Z

Loadtests. Add a test in test/load/targets.http that uses the alias to see the impact of this new logic. This should be done in another PR.

Do you mean to test how it's currently working on main and to check the impact of this PR?

Just a note: The tests for the loadtest are always taken from the head branch. I.e., if you add a test in this PR, you will run it on all 3 branches under test, so you don't need a separate PR to get results from the new test.

steve-chavez · 2026-06-02T18:39:38Z

Do you mean to test how it's currently working on main and to check the impact of this PR?

@laurenceisla I meant adding a new request like /x?select=alias:table(*)&alias.id=eq.1 (2 or 3 aliased filters would be good) in test/load/targets.http, to check if the additional logic we add to make the fix conditional has an impact on performance.

laurenceisla · 2026-06-03T17:05:27Z

I added a loadtest and here are the results: https://github.com/PostgREST/postgrest/actions/runs/26861796165. It cannot compare to 200 HTTP responses in main because that request fails in main. However, compared to 14.12 it appears to be a slight increase but I think it's negligible, right? (a couple to a few μs).

steve-chavez · 2026-06-03T19:17:35Z

However, compared to 14.12 it appears to be a slight increase but I think it's negligible, right? (a couple to a few μs).

Right, I think there's no other way anyway, we'll also remove this logic on the next major. Then the slight perf drop will go away.

@laurenceisla Could you rebase this PR so the revert and 533dec4 are no longer included? We don't need them right?

laurenceisla · 2026-06-04T01:16:42Z

@laurenceisla Could you rebase this PR so the revert and 533dec4 are no longer included? We don't need them right?

Correct. I found it difficult to separate the "new config" commit from the "emit warning" commit without the whole "revert"->"readd" thing, so I squashed them into a single one. In fact maybe all of the commits in this PR should be squashed when merged?

steve-chavez · 2026-06-04T17:05:02Z

@laurenceisla Yes, can you squash? 🙏 We've disabled squash/merge from github UI.

wolfgangwalther · 2026-06-04T17:10:56Z

+  o@LegacyTargetNameWarningObs {} ->
+    when (logLevel >= LogError) $ do
+      logWithZTime loggerState $ observationMessages o


Why are we logging a warning on log level error? That doesn't make sense.

@wolfgangwalther I thought you agreed above that this was fine?

I don't see any other way to communicate to an admin that some requests will fail on a next version. My read is that these are critical. Perhaps we should remove the WARNING: prefix?

I agreed to emitting a warning. But a warning is emitted on log-level=warn, ofc?

How can this thing be critical, if the request actually succeeds? It really is just a warning, nothing else.

But a warning is emitted on log-level=warn, ofc?

The point is to communicate to every user about the upcoming breaking change, if we cannot make log-level=warn the default (#4904), then this is not effective.

How can this thing be critical, if the request actually succeeds?

Because it will fail on a next postgREST upgrade.

What other options do we have to communicate this?

What other options do we have to communicate this?

The release notes are the canonical way to communicate this. You can hint in the release notes that this is deprecated now and if people are unsure whether they depend on it, they can enable log-level=warn to see the relevant warnings.

I don't think we should go at this assuming we can communicate this to users who do not look at the release notes at all. Those are very likely not going to look at their logs either, no matter on which level you log that stuff.

Or, to put differently: I regularly look at the release notes of things when I update. I only look at the logs when something is already broken.

they can enable log-level=warn to see the relevant warnings.

The problem really is that 4xx are also logged on warn, if these 4xx are useless for admins (discussed before), then maybe we should not log them at all? We could move them to the info level and have warn as default. Then we can make deprecations more useful and log these WARNINGs when they happen. WDYT?

Adds `url_use_legacy_target_names` config which is enabled by default. It logs a WARNING when target names are used in filters instead of its alias, e.g. `table?select=alias:target(*)&target.id=eq.1`. When it's disabled, the previous example will return an error. It will only succeed if the alias is used, and it won't log the warning anymore. This is DEPRECATED and the disabled behavior will be the default for future releases.

steve-chavez mentioned this pull request May 9, 2026

fix: unexpected results when embedding the same table twice #4104

Merged

steve-chavez commented May 9, 2026

View reviewed changes

steve-chavez mentioned this pull request May 9, 2026

Unexpected results when embedding and filtering the same table twice #4075

Open

wolfgangwalther reviewed May 24, 2026

View reviewed changes

Comment thread src/PostgREST/Config.hs Outdated

laurenceisla force-pushed the legacy-target-names branch 5 times, most recently from afd0eb5 to 78bf1e5 Compare June 1, 2026 23:30

laurenceisla reviewed Jun 1, 2026

View reviewed changes

Comment thread CHANGELOG.md

laurenceisla marked this pull request as ready for review June 1, 2026 23:51

steve-chavez commented Jun 2, 2026

View reviewed changes

Comment thread CHANGELOG.md

steve-chavez commented Jun 2, 2026

View reviewed changes

Comment thread CHANGELOG.md

laurenceisla force-pushed the legacy-target-names branch from 78bf1e5 to 2c00085 Compare June 3, 2026 03:25

laurenceisla force-pushed the legacy-target-names branch from 2c00085 to cbb508a Compare June 4, 2026 01:08

wolfgangwalther reviewed Jun 4, 2026

View reviewed changes

laurenceisla force-pushed the legacy-target-names branch from cbb508a to 894cccf Compare June 4, 2026 17:49

Uh oh!

Conversation

steve-chavez commented May 9, 2026 • edited by laurenceisla Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

steve-chavez May 9, 2026

Choose a reason for hiding this comment

Uh oh!

laurenceisla commented May 10, 2026

Uh oh!

steve-chavez commented May 10, 2026

Uh oh!

wolfgangwalther commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurenceisla commented May 11, 2026

Uh oh!

steve-chavez commented May 20, 2026

Uh oh!

steve-chavez commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented May 21, 2026

Uh oh!

Uh oh!

laurenceisla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wolfgangwalther commented Jun 2, 2026

Uh oh!

Uh oh!

Uh oh!

steve-chavez commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurenceisla commented Jun 3, 2026

Uh oh!

steve-chavez commented Jun 3, 2026

Uh oh!

laurenceisla commented Jun 4, 2026

Uh oh!

steve-chavez commented Jun 4, 2026

Uh oh!

wolfgangwalther Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

steve-chavez Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

wolfgangwalther Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

steve-chavez Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

wolfgangwalther Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

steve-chavez Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

steve-chavez commented May 9, 2026 •

edited by laurenceisla

Loading

wolfgangwalther commented May 11, 2026 •

edited

Loading

steve-chavez commented May 21, 2026 •

edited

Loading

steve-chavez commented Jun 2, 2026 •

edited

Loading