-
Notifications
You must be signed in to change notification settings - Fork 140
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
INFO message injected in JSON response #407
Comments
Thanks, I'll have a look! |
Hi, having the same issue here on the This problem occurs on 0.69, but 0.67 works fine |
That sounds like a different error. Do you have multiple signal-cli-rest-api docker containers running? |
nope, definitely had only one instance running |
Hi @bbernhard @kxait, I've seen these errors in two cases with Home Assistant :
Ko with "platform group" like :
I think that in this case HA makes 2 requests too quickly. I fixed the error by creating a group signal directly, end then replace my old group by :
That's what I've noticed related to :
|
It seems that this is not an issue with lc.getLogger("org.asamk").setLevel(verboseLevel > 1 ? Level.ALL : verboseLevel > 0 ? Level.DEBUG : Level.INFO); I think that in my case this was caused by a receive and send command running simulatenously (my application receives messages in a loop), so the lock was unavailable for a bit. |
Yeah, that's exactly the issue. The workaround is what I've implemented now (if you want to give it a try, have a look at the pre-release from yesterday). But of course, that's just a hacky solution that might break at some point. @kxait could you maybe let me know when the PR is merged? That would be great. Many thanks! |
@kxait would it maybe possible to only strip log those messages from the stdout/stderr output, but still log them to a file? The signal-cli command line already has a flag which allows to log messages to a file, but unfortunately it still prints them to stdout/stderr. Not sure if that's something you can/want to tackle in your PR, but if you do, it would be greatly appreciated! |
@bbernhard it seems the issue is actually here: combinedOutput := stdoutBuffer.String() + stderrBuffer.String() all logs are printed to stderr, and in this case we're returning the combined output of stdout and stderr, a fix to this seems to actually be to only return stdout |
This was done on purpose (see #404) |
Right, so perhaps the actual fix would be for |
I think the info and warn messages are also important (as they are often an indication that something is not working as intended or is going to break in the future). But reliably parsing those messages in an automated way is a bit tricky and cumbersome. I don't know the internals of signal-cli well enough, but what I would find great is, if there would be a commandline option that redirects that informational output to a log file. We could then expose the contents of that file via REST. So whenever there is a problem, we could tell people to have a look at that file first if there are some indications of whats going wrong. What do you think about that? Do you think that makes sense? |
Yes, that would make sense, but it would also require to break the fix for #403
There is the |
yeah, you are right. That would work then. @roschaefer would that be fine for you? I think it is a more robust solution than parsing the info and warn messages in the response data. |
@bbernhard I don't have a strong opinion here as I haven't looked into this for a while. Is the only problem that we don't escape a JSON injection? If so, couldn't we just do that? It should always be a good idea, not only for JSON but also for HTML, SQL etc. |
not necessarily, some messages are received raw
@bbernhard perhaps instead of an endpoint for the log file, we could include optional |
Yeah, that sounds much nicer. Especially, as you get really meaningful information back from signal-cli, that most of the time is useful to one in the exact moment. Really like the idea! I would even make that not configureable and always return that information (or make it configureable and make the default |
For some endpoints including additional information would introduce breaking changes:
... and many more. It could be nice to have the additional info standardized across all responses and returning an object like Another idea I had is to discard stderr from the response completely if a query param like |
The problem
Upon receiving a batch of messages, sometimes I get JSON which also contains an INFO message like this:
INFO IncomingMessageHandler - Ignoring a group message from an unauthorized sender (no member or admin)
which makes the JSON invalid.
Are you using the latest released version?
Have you read the troubleshooting page?
What type of installation are you running?
signal-cli-rest-api Docker Container
In which mode are you using the docker container?
Native Mode
What's the architecture of your host system?
x86-64
Additional information
No response
The text was updated successfully, but these errors were encountered: