AkkomaGang/akkoma
AkkomaGang/akkoma
16
67
Fork 105
Code Issues 167 Pull requests 13 Projects 2 Releases 19 Packages Wiki Activity

fix: Return the webfinger host as the instancev1 uri property #908

Closed
nikclayton wants to merge 1 commit from nikclayton/akkoma:fix-uri into develop
pull from: nikclayton/akkoma:fix-uri
merge into: AkkomaGang:develop
Conversation 4 Commits 1 Files changed 1 +1 -1
nikclayton commented 2025-05-06 09:23:16 +00:00
First-time contributor
Copy link

Despite the property name this is a domain, not a URI.

Return the WebFinger host to match Mastodon documented behaviour, public behaviour, and the behaviour of other Mastodon API compatible servers.

Fixes #907

Despite the property name this is a domain, not a URI. Return the WebFinger host to match Mastodon documented behaviour, public behaviour, and the behaviour of other Mastodon API compatible servers. Fixes #907
nikclayton added 1 commit 2025-05-06 09:23:16 +00:00
fix: Return the domain as the instancev1 uri property
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
Details
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
Details
ci/woodpecker/pr/docs Pipeline is pending approval
Details
ci/woodpecker/pr/lint Pipeline is pending approval
Details
ci/woodpecker/pr/test/1 Pipeline is pending approval
Details
ci/woodpecker/pr/test/2 Pipeline is pending approval
Details
ci/woodpecker/pull_request_closed/lint Pipeline was successful
Details
ci/woodpecker/pull_request_closed/test/2 Pipeline was successful
Details
ci/woodpecker/pull_request_closed/test/1 Pipeline was successful
Details
ci/woodpecker/pull_request_closed/build-amd64 Pipeline was successful
Details
ci/woodpecker/pull_request_closed/build-arm64 Pipeline was successful
Details
ci/woodpecker/pull_request_closed/docs Pipeline was successful
Details
77fe929948 
Despite the property name this is a domain, not a URI.

Fixes #907
Oneric reviewed 2025-05-06 15:04:19 +00:00
lib/pleroma/web/mastodon_api/views/instance_view.ex Outdated
@ -15,3 +15,3 @@
%{
uri: Pleroma.Web.Endpoint.url(),
uri: Pleroma.Web.WebFinger.host(),
Oneric commented 2025-05-06 15:04:19 +00:00
Owner
Copy link

This change actually does more than just removing the protocol identifier.
Endpoint.url() is the domain used for ActivityPub IDs and REST API, Webfinger.host() the domain used in userhandles and they can differ; e.g. having an endpoint of social.example.org but userhandle directly on example.org isn’t too uncommon.

I checked a GtS instance with such a setup and it does indeed use the WebFinger domain, but I don’t know any such Mastodon instance. Given your goal is to exactly match Mastodon, could you check and make sure that’s really what Mastodon wants here?

And if this turns out we want this change, please mention this change and why we want it in the commit message

This change actually does more than just removing the protocol identifier. `Endpoint.url()` is the domain used for ActivityPub IDs and REST API, `Webfinger.host()` the domain used in userhandles and they can differ; e.g. having an endpoint of `social.example.org` but userhandle directly on `example.org` isn’t too uncommon. I checked a GtS instance with such a setup and it does indeed use the WebFinger domain, but I don’t know any such Mastodon instance. Given your goal is to exactly match Mastodon, could you check and make sure that’s really what Mastodon wants here? And if this turns out we want this change, please mention this change and why we want it in the commit message
nikclayton commented 2025-05-06 20:03:32 +00:00
Author
First-time contributor
Copy link

Done.

As an example, https://social.vivaldi.net/ -- social.vivaldi.net is the host, they user vivaldi.net as the server (e.g., https://social.vivaldi.net/@JaykeBird is @jaykebird@vivialdi.net (chosen at random)).

https://social.vivaldi.net/api/v1/instance reports uri as vivaldi.net and https://social.vivaldi.net/api/v2/instance reports domain as vivaldi.net.

Done. As an example, https://social.vivaldi.net/ -- `social.vivaldi.net` is the host, they user `vivaldi.net` as the server (e.g., https://social.vivaldi.net/@JaykeBird is @jaykebird@vivialdi.net (chosen at random)). https://social.vivaldi.net/api/v1/instance reports `uri` as `vivaldi.net` and https://social.vivaldi.net/api/v2/instance reports `domain` as `vivaldi.net`.
Oneric commented 2025-05-08 12:13:03 +00:00
Owner
Copy link

Thanks for confirming!

Unfortunately you appear to have forgot to actually change the commit message and merely (closed&reopened and) renamed the ull request title.

You can do so with git commit --amend followed by git push --force-with-lease — or just set the toggle in the sidebar to allow maintainer edits then I can fix it up before merge myself.

Ideally the commit message would be something like:

api/masto/instance: use bare WebFinger domain for uri

Despite its name this property is not supposed to be a full URI,
but just a bare domain witout protocol. Additionally (and not mentioned
in Masto docs), it’s supposed to be the WebFinger domain used in
userhandles and NOT the domain used for API and ActivityPub objects.

Akkoma’s divergence here is causing troubles in Pachli and Tusky.

Fixes: https://akkoma.dev/AkkomaGang/akkoma/issues/907
Thanks for confirming! Unfortunately you appear to have forgot to actually change the commit message and merely (closed&reopened and) renamed the ull request title. You can do so with `git commit --amend` followed by `git push --force-with-lease` — or just set the toggle in the sidebar to allow maintainer edits then I can fix it up before merge myself. Ideally the commit message would be something like: ``` api/masto/instance: use bare WebFinger domain for uri Despite its name this property is not supposed to be a full URI, but just a bare domain witout protocol. Additionally (and not mentioned in Masto docs), it’s supposed to be the WebFinger domain used in userhandles and NOT the domain used for API and ActivityPub objects. Akkoma’s divergence here is causing troubles in Pachli and Tusky. Fixes: https://akkoma.dev/AkkomaGang/akkoma/issues/907 ```
nikclayton commented 2025-05-08 12:23:48 +00:00
Author
First-time contributor
Copy link

Maintainer edits has been toggled. Please go ahead.

One point. "Akkoma’s divergence here is causing troubles in Pachli and Tusky." is irrelevant and quite possibly incorrect. I do not know which other clients / tools may be affected by this.

Maintainer edits has been toggled. Please go ahead. One point. "Akkoma’s divergence here is causing troubles in Pachli and Tusky." is irrelevant and quite possibly incorrect. I do not know which other clients / tools may be affected by this.
Oneric commented 2025-05-08 12:25:54 +00:00
Owner
Copy link

Then please clarify what you meant with this over in #907 if not "causing troubles for Pachli and Tusky" and/or which client or app you’re working on

I know that Pachli (and by inheritance, Tusky) have code that is more complicated than it needs to be to work around this Akkoma bug.

Then please clarify what you meant with this over in #907 if not "causing troubles for Pachli and Tusky" and/or which client or app you’re working on > I know that Pachli (and by inheritance, Tusky) have code that is more complicated than it needs to be to work around this Akkoma bug.
nikclayton commented 2025-05-08 12:38:45 +00:00
Author
First-time contributor
Copy link
  1. It's irrelevant to the commit message -- this is a bug that should be fixed irrespective of whether or not it's actively causing problems for clients.
  2. As written it implies those are the only clients affected by this. I have no way of knowing if the issue is restricted to just those two, so including it in the commit message is misleading, as other clients / tools may also be affected.

Obviously, your project, your preferences for commit messages, but if the Akomma project's position is that deviations from the spec are only worth correcting if they're known to cause problems then I view that as a red flag (see also the initial push back on #907) for the maturity of the project's development practices.

Compare and contrast with the attitude of the Sharkey project. Initial report in https://activitypub.software/TransFem-org/Sharkey/-/issues/1046, no back-and-forth about whether or not this actually needs to be fixed, they just fixed it (https://activitypub.software/TransFem-org/Sharkey/-/merge_requests/980).

Again, your project, your preferences, so feel free to do whatever you want with this feedback.

1. It's irrelevant to the commit message -- this is a bug that should be fixed irrespective of whether or not it's actively causing problems for clients. 2. As written it implies those are the only clients affected by this. I have no way of knowing if the issue is restricted to just those two, so including it in the commit message is misleading, as other clients / tools may also be affected. Obviously, your project, your preferences for commit messages, but if the Akomma project's position is that deviations from the spec are only worth correcting if they're known to cause problems then I view that as a red flag (see also the initial push back on #907) for the maturity of the project's development practices. Compare and contrast with the attitude of the Sharkey project. Initial report in https://activitypub.software/TransFem-org/Sharkey/-/issues/1046, no back-and-forth about whether or not this actually needs to be fixed, they just fixed it (https://activitypub.software/TransFem-org/Sharkey/-/merge_requests/980). Again, your project, your preferences, so feel free to do whatever you want with this feedback.
Oneric commented 2025-05-08 13:47:07 +00:00
Owner
Copy link

it implies those are the only clients affected by this

I do not see any such implication, but regardless I changed the wording a bit.

The motivation for a change and the relevant real-life test cases are quite important to understand the change itself and as a reference in case further adjustments are needed in the future.

> it implies those are the only clients affected by this I do not see any such implication, but regardless I changed the wording a bit. The motivation for a change and the relevant real-life test cases are quite important to understand the change itself and as a reference in case further adjustments are needed in the future.
Oneric marked this conversation as resolved
nikclayton closed this pull request 2025-05-06 20:00:16 +00:00
nikclayton reopened this pull request 2025-05-06 20:00:18 +00:00
nikclayton changed title from fix: Return the domain as the instancev1 uri property to fix: Return the webfinger host as the instancev1 uri property 2025-05-06 20:05:16 +00:00
Oneric force-pushed fix-uri from 77fe929948 to b54d98dba8 2025-05-08 12:29:45 +00:00 Compare
Oneric force-pushed fix-uri from b54d98dba8 to 707ef25b56 2025-05-08 13:43:24 +00:00 Compare
Oneric requested changes 2025-05-08 14:00:39 +00:00
Oneric left a comment
Owner
Copy link

CI fails with new build warnings and a bunch of failing tests due to attempting to use a non-existent function.
Please always run tests and make sure changes actually work before submitting a patch. A defunct change just creates more delays and work for everyone involved.

I’d also advise against keeping up needless antagonism and refusal to cooperate; neither is helpful. It’s understandable to not immediately provide all info if things seems obvious to you, but then going out of your way to explicitly not answer follow up questions as done initially in the accompanying issue is quite unproductive. Having a bad day once is acceptable; keeping it up is not.

[CI fails](https://ci.akkoma.dev/repos/1/pipeline/162) with new build warnings and a bunch of failing tests due to attempting to use a non-existent function. Please always run tests and make sure changes actually work before submitting a patch. A defunct change just creates more delays and work for everyone involved. I’d also advise against keeping up needless antagonism and refusal to cooperate; neither is helpful. It’s understandable to not immediately provide all info if things seems obvious to you, but then going out of your way to explicitly not answer follow up questions as done initially in the accompanying issue is quite unproductive. Having a bad day once is acceptable; keeping it up is not.
lib/pleroma/web/mastodon_api/views/instance_view.ex
@ -15,3 +15,3 @@
%{
uri: Pleroma.Web.Endpoint.url(),
uri: Pleroma.Web.WebFinger.host(),
Oneric commented 2025-05-08 13:48:47 +00:00
Owner
Copy link

No host method exists neither as a public nor private function and I have no idea how you arrived at this name.
You probably meant to use ´domain`

No `host` method exists neither as a public nor private function and I have no idea how you arrived at this name. You probably meant to use ´domain`
nikclayton commented 2025-05-09 10:41:14 +00:00
Author
First-time contributor
Copy link

  1. The PR was generated through the web UI (https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/lib/pleroma/web/mastodon_api/views/instance_view.ex, click the "edit" button). Please consider either (a) enabling CI runs on PRs generated this way, so potential contributors who do not have a local checkout of the code can easily see test results, or (b) if you require PR contributors to run local tests first then disable this mechanism for creating PRs.

  2. "I have no idea how you arrived at this name." -- it's what Pleroma uses, so it seemed reasonable it would either work, or be a sufficient starting point for someone else who knows the Akkoma code base (https://git.pleroma.social/pleroma/pleroma/-/blob/develop/lib/pleroma/web/mastodon_api/views/instance_view.ex?ref_type=heads).

  3. It is antagonistic, when presented with a clear deviation from a spec, to do anything other than plan work to fix the deviation. Questions like "does including it cause any issues" and "do there exist clients which fail on this specific deviation" are both irrelevant for deciding whether to fix this, inherently unanswerable without surveying all possible API clients, and come across as attempts to dodge responsibility for fixing the problem.

Again, I commend the Sharkey project's approach to you when faced with the same issue -- a clear understanding that deviation from the spec is their responsibility to fix, and a swift fix.

1. The PR was generated through the web UI (https://akkoma.dev/AkkomaGang/akkoma/src/branch/develop/lib/pleroma/web/mastodon_api/views/instance_view.ex, click the "edit" button). Please consider either (a) enabling CI runs on PRs generated this way, so potential contributors who do not have a local checkout of the code can easily see test results, or (b) if you require PR contributors to run local tests first then disable this mechanism for creating PRs. 2. "I have no idea how you arrived at this name." -- it's what Pleroma uses, so it seemed reasonable it would either work, or be a sufficient starting point for someone else who knows the Akkoma code base (https://git.pleroma.social/pleroma/pleroma/-/blob/develop/lib/pleroma/web/mastodon_api/views/instance_view.ex?ref_type=heads). 3. It is antagonistic, when presented with a clear deviation from a spec, to do anything other than plan work to fix the deviation. Questions like "does including it cause any issues" and "do there exist clients which fail on this specific deviation" are both irrelevant for deciding whether to fix this, inherently unanswerable without surveying all possible API clients, and come across as attempts to dodge responsibility for fixing the problem. Again, I commend the Sharkey project's approach to you when faced with the same issue -- a clear understanding that deviation from the spec is their responsibility to fix, and a swift fix.
Oneric commented 2025-05-09 12:40:12 +00:00
Owner
Copy link

It is not possible to disable the UI editor. Commits which don’t build or pass tests are never acceptable and ensuring the work they present is not utterly broken and reasonably likely to be up to the task is always the author’s responsibility.
Bad choices, like using an editor without using any of the available sanity checks and blindly copying code excerpts from some other project (without attribution even), being in theory possible does not deflect the responsibility of choosing to make them.

The process of how to contribute and what's expected is documented.

Describing an issue and suggesting a fix is not antagonistic, refusing cooperation in determining the severity of the problem and understanding how the field is used by clients, throwing shade at every opportunity and wasting time with entirely made-up, broken changes is.

If you do not understand this, I suggest you retreat from this matter as it doesn’t help your goal of seeing this fixed.
If you had just stopped after reporting the issue and telling us the real-world impact, it would likely already be fixed by now; i spent more time on dealing with this than it takes to create and test a fix from scratch.

It is [not possible](https://codeberg.org/forgejo/forgejo/issues/5290) to disable the UI editor. Commits which don’t build or pass tests are never acceptable and ensuring the work they present is not utterly broken and reasonably likely to be up to the task is _always_ the author’s responsibility. Bad choices, like using an editor without using any of the available sanity checks and blindly copying code excerpts from some other project *(without attribution even)*, being in theory possible does not deflect the responsibility of choosing to make them. The process of how to contribute and what's expected is documented. Describing an issue and suggesting a fix is not antagonistic, refusing cooperation in determining the severity of the problem and understanding how the field is used by clients, throwing shade at every opportunity and wasting time with entirely made-up, broken changes is. If you do not understand this, I suggest you retreat from this matter as it doesn’t help your goal of seeing this fixed. If you had just stopped after reporting the issue and telling us the real-world impact, it would likely already be fixed by now; i spent more time on dealing with this than it takes to create and test a fix from scratch.
floatingghost commented 2025-05-09 16:52:10 +00:00
Owner
Copy link

whilst i commend your desire to conform to specification, i would just like to reiterate that mastodon's docs are not a spec! they have in the past changed how fields behave, and likely will again. they never intended for anyone else to actually implement an API using the same schema, but people did

as such, drift between implementations will happen,

whilst i commend your desire to conform to specification, i would just like to reiterate that mastodon's docs are not a spec! they have in the past changed how fields behave, and likely will again. they never intended for anyone else to actually implement an API using the same schema, but people did as such, drift between implementations *will* happen,
Oneric commented 2025-05-15 19:12:27 +00:00
Owner
Copy link

replaced by #927

replaced by #927
Oneric closed this pull request 2025-05-15 19:12:27 +00:00
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
Details
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
Details
ci/woodpecker/pr/docs Pipeline is pending approval
Details
ci/woodpecker/pr/lint Pipeline is pending approval
Details
ci/woodpecker/pr/test/1 Pipeline is pending approval
Details
ci/woodpecker/pr/test/2 Pipeline is pending approval
Details
ci/woodpecker/pull_request_closed/lint Pipeline was successful
Details
ci/woodpecker/pull_request_closed/test/1 Pipeline was successful
Details
ci/woodpecker/pull_request_closed/test/2 Pipeline was successful
Details
ci/woodpecker/pull_request_closed/build-arm64 Pipeline was successful
Details
ci/woodpecker/pull_request_closed/build-amd64 Pipeline was successful
Details
ci/woodpecker/pull_request_closed/docs Pipeline was successful
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Oneric
Labels
Clear labels   
approved, awaiting change

   
bug

Something is not working

  
configuration

This requires config changes

  
documentation

This is about human-readable docs

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
extremely low priority

   
feature request

Something new?

  
Fix it yourself

Support has been attempted

  
help wanted

Need some help

  
invalid

Something is wrong

  
mastodon_api

   
needs change/feedback

Issue or PR is stalled on response from the author or a third party

  
needs docs

   
needs tests

   
not a bug

   
planned

   
pleroma_api

   
privacy

   
question

More information is needed

  
static_fe

   
triage

   
wontfix

This won't be fixed

No labels
approved, awaiting change
bug
configuration
documentation
duplicate
enhancement
extremely low priority
feature request
Fix it yourself
help wanted
invalid
mastodon_api
needs change/feedback
needs docs
needs tests
not a bug
planned
pleroma_api
privacy
question
static_fe
triage
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: AkkomaGang/akkoma#908
No description provided.
Delete branch "nikclayton/akkoma:fix-uri"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?