Skip to content

Make REST endpoints accept more logger name characters - #4983

Merged
holgerfriedrich merged 1 commit into
openhab:mainfrom
Nadahar:fix-rest-logger-name
Sep 14, 2025
Merged

Make REST endpoints accept more logger name characters#4983
holgerfriedrich merged 1 commit into
openhab:mainfrom
Nadahar:fix-rest-logger-name

Conversation

@Nadahar

@Nadahar Nadahar commented Aug 26, 2025

Copy link
Copy Markdown
Contributor

This fixes #4946.

Given that both _ and - are otherwise in use as logger names in various places in the system, they should be allowed in the REST endpoints so that all loggers can be accessed via the REST API. This PR makes the necessary changes.

@Nadahar
Nadahar requested a review from a team as a code owner August 26, 2025 15:11
@rkoshak

rkoshak commented Aug 26, 2025

Copy link
Copy Markdown

Are spaces allowed? I've certainly see users use spaces in logger names inside rules before.

@Nadahar

Nadahar commented Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Are spaces allowed? I've certainly see users use spaces in logger names inside rules before.

I haven't added spaces - it could be done if it doesn't cause issues. I simply considered - and _ quite "safe" after some quick checking around.

Does anybody have any idea if allowing spaces would be problematic?

@Nadahar
Nadahar marked this pull request as draft August 26, 2025 21:49
@Nadahar

Nadahar commented Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

I'm converting this to draft temporarily because I discovered one more change that's needed to be able to set the log level for loggers with - in the name, plus that it would be nice to figure out if space should be included as well.

@Nadahar
Nadahar force-pushed the fix-rest-logger-name branch from bec80b1 to 97b4186 Compare August 27, 2025 00:05
@Nadahar

Nadahar commented Aug 27, 2025

Copy link
Copy Markdown
Contributor Author

@rkoshak I'm unsure of how to proceed. Not a lot of feedback to be gotten it seems, I can include space too, I'm just worried that it might have some side effects that I haven't thought about. But, if you're saying that users already use logger names with spaces, that indicates that it doesn't cause a problem.

"Normally" logger names are Java class names, which can't contain spaces. Thus, I'm a bit unsure if everything is "made to work" with spaces in the logger names. But, most likely, they are just strings, and the content of the string doesn't really matter to the logging framework.

@rkoshak

rkoshak commented Aug 27, 2025

Copy link
Copy Markdown

I can say it doesn't cause a problem to write to the logs. If that works I see no reason why setting the logging level won't work too but I've never done it.

But I can confirm people use spaces in logger names, particularly with Rules DSL where the first argument to all log statements is the logger name. I've seen many examples of people including spaces in that first argument.

logInfo('Logger Name', 'Some logging statement')

@Nadahar
Nadahar force-pushed the fix-rest-logger-name branch from 97b4186 to f02694e Compare August 27, 2025 21:21
@Nadahar

Nadahar commented Aug 27, 2025

Copy link
Copy Markdown
Contributor Author

Space is now included and working according to my testing.

@Nadahar
Nadahar marked this pull request as ready for review August 27, 2025 21:22
Signed-off-by: Ravi Nadahar <nadahar@rediffmail.com>
@Nadahar
Nadahar force-pushed the fix-rest-logger-name branch from f02694e to 5a9e155 Compare August 27, 2025 23:49
@wborn
wborn requested a review from Copilot August 31, 2025 10:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies REST endpoint path patterns to allow additional characters in logger names, specifically underscore and hyphen characters. The change enables all loggers in the system to be accessible via the REST API by expanding the allowed character set in both validation patterns and JAX-RS path expressions.

Key Changes:

  • Updated regex pattern for logger name validation to include hyphens and spaces
  • Modified JAX-RS path patterns to accept underscores, hyphens, and URL-encoded spaces in logger names
  • Applied changes consistently across GET, PUT, and DELETE endpoints

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@lsiepel lsiepel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. @openhab/core-maintainers

@holgerfriedrich holgerfriedrich left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am wondering why handle the first character differently (or I missed to decode the regex correctly ;-) ).
\w is more restrictive as before (I know, edge case, but . was a valid match before and is now rejected.)

Was this intentional?

@Nadahar

Nadahar commented Sep 14, 2025

Copy link
Copy Markdown
Contributor Author

I am wondering why handle the first character differently (or I missed to decode the regex correctly ;-) ). \w is more restrictive as before (I know, edge case, but . was a valid match before and is now rejected.)

Was this intentional?

Yes, it was intentional because as far as I know, it's not actually a valid logger name if it starts with a . (. is the separator, not an "element" in itself).

@Nadahar

Nadahar commented Sep 14, 2025

Copy link
Copy Markdown
Contributor Author

I can just add that the reason two different regexes are in use, is that one of for the URL encoded version and the other is for the "decoded" version.

@holgerfriedrich holgerfriedrich left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You comment is fine for me. I don't see a use case for a logger name starting with . as well.
Maybe we get a bug report for this later on because we missed something 🙈

@holgerfriedrich
holgerfriedrich merged commit e39fb12 into openhab:main Sep 14, 2025
4 checks passed
@holgerfriedrich holgerfriedrich added the enhancement An enhancement or new feature of the Core label Sep 14, 2025
@holgerfriedrich holgerfriedrich added this to the 5.1 milestone Sep 14, 2025
@Nadahar

Nadahar commented Sep 14, 2025

Copy link
Copy Markdown
Contributor Author

Maybe we get a bug report for this later on because we missed something 🙈

That's always a possibility 😉

@Nadahar
Nadahar deleted the fix-rest-logger-name branch September 14, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement An enhancement or new feature of the Core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

REST access to /logging/{loggerName} returns 404 for valid logger containing non-alpha chars (eg _, -)

5 participants