Skip to content
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

fix: injected standard prefixes reference wikidata #804

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

rti
Copy link
Contributor

@rti rti commented Nov 8, 2024

As reported in #798, default prefixes injected by WDQS did not align with the WIKIBASE_CONCEPT_URI setting added in #771. This PR fixes it.

As reported in #798, the "standard prefixes" injected by WDQS do not align with the WIKIBASE_CONCEPT_URI setting introduced in #771, nor do they match the corresponding values for Wikidata. Currently, the WDQS-injected "standard prefixes" (such as wd:) reference http://wikidata/, which is the internal hostname of the wikibase/mediawiki container within the Docker network.

Particularly in the context of federation, it is essential to maintain the "standard prefixes" for referencing Wikidata [1] [2]. To accommodate the local Wikibase instance, prefixes can be set inline, as is currently done on wikibase.cloud [2]. This pull request addresses this discrepancy and updates the "standard prefixes" to point to Wikidata. Additional context can be found in [3].

[1] https://www.mediawiki.org/wiki/Wikibase/Wikibase.cloud/First_steps#View_your_data_using_the_Query_Service
[2] https://phabricator.wikimedia.org/T335448
[3] https://phabricator.wikimedia.org/T379232

@rti rti force-pushed the fix-wdqs-std-prefixes-use-concept-uri branch from cb805bf to 4e5589b Compare November 11, 2024 08:51
@rti rti force-pushed the fix-wdqs-std-prefixes-use-concept-uri branch from 4e5589b to 11fb8b2 Compare November 11, 2024 08:51
darthmon
darthmon previously approved these changes Nov 11, 2024
Copy link
Contributor

@darthmon darthmon left a comment

Choose a reason for hiding this comment

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

it all looks good to me :)

@tarrow
Copy link
Contributor

tarrow commented Nov 11, 2024

Unfortunately I've definitely not managed to build all the context of this ticket up in my head and I can't dedicate the time to doing it justice but let me try to ask a couple of maybe helpful questions:

  • Did you try this and find it then behaved as expected/desired by the product manager?
  • Is it perhaps the case that introducing the WIKIBASE_HOST_PROPERTY at some point in the past was actually the cause of the breakage of this functionality and that this is now the wrong way to fix it? Would you not actually want to go back to the forWikidataHost(WIKIDATA_HOSTNAME); as the UriScheme?
  • Could you unset the WIKIBASE_HOST_PROPERTY instead to return to the old functionality?

Nothing blocking and perhaps nothing smart said but maybe it's of some help. :)

@rti
Copy link
Contributor Author

rti commented Nov 12, 2024

Thanks for your thoughts @tarrow, maybe this is not the best idea, even as a temporary solution. Thanks for catching that. Interestingly, the use of wikibaseHost goes back to 2021 to the birth of "example" (78f1acf).

@tarrow
Copy link
Contributor

tarrow commented Nov 12, 2024

Yeah, TBH I think there was never a proper specification for how this should all behave.

I also wondered if the WIKIBASE_HOST_PROPERTY is important to be set for some other functionality (like the label service) or something.

I'd actually question if "magically injecting prefixes" was ever a desirable thing from a product standpoint. It saves a tiny bit of typing or clicking a "add prefixes" button in some UI at the expense of some real mystery for the user.

I skimmed the SPARQL spec for how acceptable or not it would be to have this magic insertion of prefixes and couldn't find any strong evidence for or against.

@rti rti requested review from darthmon and tarrow November 13, 2024 13:16
@rti
Copy link
Contributor Author

rti commented Nov 13, 2024

@darthmon @tarrow please have a look on the latest version of this PR. WDYT?

@rti rti changed the title fix: use WIKIBASE_CONCEPT_URI for default prefixes fix: injected standard prefixes reference wikidata Nov 13, 2024
@rti rti dismissed darthmon’s stale review November 13, 2024 13:35

solution has been completely changed, please have a look again.

Copy link
Contributor

@tarrow tarrow left a comment

Choose a reason for hiding this comment

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

LGTM but I'm now also hoping that this doesn't impact in some way the "label" SERVICE; I wonder if you already checked this?

@tarrow
Copy link
Contributor

tarrow commented Nov 18, 2024

I was merely commenting and not requesting changes/approval) because I'd not care to tread on any toes of people not in my team :)

@rti rti requested a review from a team November 25, 2024 09:33
Copy link
Contributor

@lorenjohnson lorenjohnson left a comment

Choose a reason for hiding this comment

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

I haven't fully explored this, but to the 70% that I do grok what is happening here this seems like a clear and needed fix. The comment is appreciated. So this won't break current configuration for folks?

@rti
Copy link
Contributor Author

rti commented Nov 27, 2024

LGTM but I'm now also hoping that this doesn't impact in some way the "label" SERVICE; I wonder if you already checked this?

Thanks for catching that. I just double checked and the label service works correctly in e.g.

PREFIX PP: <https://wikibase.example/prop/>

SELECT ?aLabel
WHERE
{
  ?a PP:P1 ?c
  SERVICE wikibase:label { bd:serviceParam wikibase:language "[AUTO_LANGUAGE]". }
}

@rti
Copy link
Contributor Author

rti commented Nov 27, 2024

So this won't break current configuration for folks?

Well, I would say: "it finally works as expected". So, "no", because I would consider this a bug fix.

@rti rti merged commit e97290e into main Nov 28, 2024
15 checks passed
@rti rti deleted the fix-wdqs-std-prefixes-use-concept-uri branch November 28, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants