-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: emit warning for legacy target names #4905
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,9 @@ observationLogger loggerState logLevel obs = case obs of | |
| o@(QueryObs _ status) -> do | ||
| when (shouldLogResponse logLevel status) $ | ||
| logWithZTime loggerState $ observationMessages o | ||
| o@LegacyTargetNameWarningObs {} -> | ||
| when (logLevel >= LogError) $ do | ||
| logWithZTime loggerState $ observationMessages o | ||
|
Comment on lines
+88
to
+90
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we logging a warning on log level error? That doesn't make sense.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The point is to communicate to every user about the upcoming breaking change, if we cannot make
Because it will fail on a next postgREST upgrade. What other options do we have to communicate this?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The problem really is that 4xx are also logged on
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're going in circles here. The 4xx are "useless" in the same way as this new warning is: This warning is not actionable for admins. The change needs to happen on the client, not the server. Imho what we really need to do is:
This would report the problem in the right place - the client, where the old syntax originates from. |
||
| o@PoolRequest -> | ||
| when (logLevel >= LogDebug) $ do | ||
| logWithZTime loggerState $ observationMessages o | ||
|
|
@@ -190,6 +193,11 @@ observationMessages = \case | |
| let snipts = renderSnippet <$> [mqTxVars, fromMaybe mempty mqPreReq, mqMain, x, y, z, fromMaybe mempty mqExplain] | ||
| in | ||
| showOnSingleLine '\n' . T.decodeUtf8 <$> filter (/= mempty) snipts | ||
| LegacyTargetNameWarningObs requestMethod requestTarget warnings -> | ||
| let replacement (relName, alias) = "`" <> relName <> "` to `" <> alias <> "`" in | ||
| [ "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." | ||
| , "Please update the filters that use " <> T.intercalate ", " (replacement <$> warnings) <> " in " <> "`" <> T.decodeUtf8 (requestMethod <> " " <> requestTarget) <> "`" | ||
| ] | ||
| ConfigReadErrorObs usageErr -> | ||
| pure $ "Failed to query database settings for the config parameters." <> jsonMessage usageErr | ||
| QueryRoleSettingsErrorObs usageErr -> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.