search

Arksine / moonraker

Web API Server for Klipper
https://moonraker.readthedocs.io
GNU General Public License v3.0
1.02k stars 392 forks source link

Fix if spoolman spool is missing #791

Closed Zeanon closed 5 months ago

Zeanon commented 5 months ago

Currently if the last selected spool gets deleted from the spoolman database, the spoolman component fails to load. This fix sets the selected spool to None if that happens so the component can load

Arksine commented 5 months ago

Hi. A 404 returned by the proxy request would not cause a failure to load the Spoolman component. The front end is misreporting it as such because it believes that the proxy endpoint doesn't exist due to the 404, when its Spoolman is returning 404. While this is understandable, I think that frontends need to treat errors as if they were returned by Spoolman when using this endpoint. Moonraker reports a list of loaded components and failed components in its /server/info endpoint.

I don't think its a good idea to change the active spool in the proxy endpoint. The message string isn't a reliable source for comparison, if the error response changes just one character we will always evaluate to False. In addition, having Moonraker return the message in place of a 404 error represents an API change that some clients may not be able to deal with.

There may be some things we can do better to help alleviate this issue, but regardless of what we do changes will need to be made at the frontend to accommodate it.

Zeanon commented 5 months ago

Fair enough, it was just a initial draft, I'm open to new ideas I figured the safest way to only detect missing spools was, to compare the error message(as that indicates that exact error) But out of curiosity: the component failed to load message you get in mainsail, is that from mainsail or moonraker? cause if it's moonraker, then moonraker does need to be changed as imo, it should not prevent the component from loading of the spool that was registered last got deleted(especiall since you have no way of changing it when you can't access the component)

Might also be a good idea to talk to the guy from spoolman and maybe add a specifiy error code for spool not found?

Arksine commented 5 months ago

I just opened #792 with the aim of resolving this issue. I makes some changes that should reduce the chance that Moonraker's spool id does not exist in the Spoolman Database. In addition it makes an API change that will allow frontends to deal with the ambiguous 404 error, however it will require implementation on their side.

But out of curiosity: the component failed to load message you get in mainsail, is that from mainsail or moonraker?

Based on the behavior described, I'm reasonably certain that the component hasn't actually failed. The ambiguous 404 returned when a spool doesn't exist translates to Method not found, so Mainsail thinks that the component isn't loaded when it is.

Might also be a good idea to talk to the guy from spoolman and maybe add a specifiy error code for spool not found?

In this case 404 is the right code to return. I think correcting this issue in Moonraker and the frontends is the appropriate solution.

Zeanon commented 5 months ago

I just opened #792 with the aim of resolving this issue. I makes some changes that should reduce the chance that Moonraker's spool id does not exist in the Spoolman Database. In addition it makes an API change that will allow frontends to deal with the ambiguous 404 error, however it will require implementation on their side.

Problem is that moonraker can't control whether the spool gets deleted. Initially the problem occured cause I created a dummy spool to test spoolman (cause I am running it on a remote server with an automated ssh tunnel so the printers can access it and so on, not important here), tldr I wanted to test stuff and after testing I delketed the dummy spool, cause why keep it, it was just a dummy for testing. Then spoolman stopped working on 2 of 3 printers while I could still access the url via terminal, so I was quite confused.

Concerning the mainsail reads error wrong, I'll try to come up with a patch for that and do a pr on mainsail, I just figured it would be a moonraker error, since I got a moonraker warning(which is actually mainsail, which is confusing XD)

And concerning the error code: I am not that much of a web dev and not that much into error codes, so my bad I guess :) I just figured opening a pr and presenting a solution is better than just complaining :)

Arksine commented 5 months ago

This should now be resolved on Moonraker's side. The major known frontends have changes complete or in process.