If there is a simple and clear solution to "disable" some requests, I could vote my approval.

But this change doesn't look good to me: too many "if" tricks and unnecessary new error type.

I prefer to be "restraint" to avoid making the code more complex than it should be.

-> #33205 (comment)

IMO it's not feasible to keep adding more options and "if" code to make every feature "disable-able".

If there is a simple and clear solution to "disable" some requests

a middleware maybe?

unnecessary new error type

it's not 400: nothing wrong with the request, the resource is just disabled
it's not 404: it does exist, but is disabled
it's not 403: user would have permission to do it if it was enabled
it's not 409: nothing user can do would make the request succeed
it's not 500: not an error, just disabled
it's not 503: it's likely not a temporary response and unrelated to the whole service

410 Gone is the status that best fits a resource that was intentionally made unavailable, likely permanently, and it did not exist already so I had to create it for Swagger.

IMO it's not feasible to keep adding more options and "if" code to make every feature "disable-able".

I get that, but having a piece of software that is highly customizable without having to hack it yourself is a very good thing. I personally want to completely disable the repo watching feature, which is not currently doable via config, so I'm having to implement it myself, and will contribute my changes in case someone else needs to disable it as well. isn't that what OSS is about?

it's not 404: it does exist, but is disabled

Gitea already uses 403/404 for disabled features, see other code.

IMO it's not feasible to keep adding more options and "if" code to make every feature "disable-able".

I get that, but having a piece of software that is highly customizable without having to hack it yourself is a very good thing. I personally want to completely disable the repo watching feature, which is not currently doable via config, so I'm having to implement it myself, and will contribute my changes in case someone else needs to disable it as well. isn't that what OSS is about?

That's a trade-off. Just like Linux doesn't accept all patches into the kernel, it depends on many facts: the public interests, the maintainability cost, the benefits to end users, etc. My opinions are:

• If it is clear and simple enough: then no maintainability cost, then look good to me.
• If it is complex (for example: many "if" blocks or new errors which are difficult to test): it needs to bring enough benefits to end users.
  • For this case: the "star" button had been hidden from UI, even some APIs are called, nothing wrong nothing bad, so I do not see real differences to end users.

Disclaimer: that's just my opinion (to explain "look good to 'me' or not"), other maintainers could still approve if it looks good to them.

I think this is a reasonable pattern to establish, as even though "stars" may not do much if someone gets around the block by using the API, in the case of watchers that has an impact due to emails being sent out.

As for status, 403 is acceptable as it's "you are forbidden from doing this", and while gone does make sense, it's for things that don't come back, but in theory stars could be re-enabled

in the case of watchers that has an impact due to emails being sent out.

and it makes total sense for stars to be consistent with watching being fully disabled.

I think this is a reasonable pattern to establish, as even though "stars" may not do much if someone gets around the block by using the API, in the case of watchers that has an impact due to emails being sent out.

That's a different problem and not in this PR's scope (unrelated)

If you would like to fix that problem, it also needs tests to make sure the logic is right and won't break.

it also needs tests to make sure the logic is right and won't break.

if you're willing to explain how to set config options during testing I'll definitely write them, but as of right now I have no idea of how to do that or if it's even possible.

test.MockVariableValue
you can find example usage here: https://github.com/go-gitea/gitea/blob/main/modules/git/url/url_test.go#L248

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/410

If server owners don't know whether this condition is temporary or permanent, a 404 status code should be used instead.

As DISABLE_STARS is an optional setting, it is hard to say it is temporary or permanent.

As DISABLE_STARS is an optional setting, it is hard to say it is temporary or permanent.

IMO it has a higher chance of being permanent, i.e. an undesired feature than temporary, i.e. "someone is abusing it so I'll turn if off for now" or something. Can't say the same for all config options though.

Plus they also state: this condition is *likely* to be permanent (emphasis mine), so it doesn't have to be.

That being said, for consistency with existing code we should either go with 403 or 404, and I personally prefer the latter.

I've been busy this week but I'll try to get more progress done in this tonight.

• moved check from the models to the routes
• return 403 Forbidden instead of 410 Gone
• regenerated Swagger template

now I think I need to tackle tests, which I'm not looking forward to.