Skip to content

SEARCH-751: Add the target-type property to relationships in event and work JSON#68

Open
mwiencek wants to merge 4 commits into
metabrainz:masterfrom
mwiencek:SEARCH-751
Open

SEARCH-751: Add the target-type property to relationships in event and work JSON#68
mwiencek wants to merge 4 commits into
metabrainz:masterfrom
mwiencek:SEARCH-751

Conversation

@mwiencek

Copy link
Copy Markdown
Member

This adds the target-type property to event and work relationships, which is present in the JSON output of musicbrainz-server.

Extending the RelationAdapter usage to areas and URLs would fix SEARCH-444, but is a larger breaking change, which is why this only applies to events and works.

Tested via manual queries on my local search server, and by adding to or updating existing tests.

@reosarevok

Copy link
Copy Markdown
Member

I am a bit confused. Is this a breaking change only if also applied to areas and URLs? Or is it a breaking change anyway but we are ok with it for events and works?

Generally for JSON we have implemented this sort of improvement when it would be a breaking change by having both the old weird way and the new corrected way together in the response, haven't we? Is there a big reason this would not work here?

The change itself seems fine AFAICT, based on the tests.

@mwiencek

Copy link
Copy Markdown
Member Author

I am a bit confused. Is this a breaking change only if also applied to areas and URLs? Or is it a breaking change anyway but we are ok with it for events and works?

The larger breaking change I was talking about for areas and URLs is SEARCH-444, not SEARCH-751. The problem is that injecting the target-type currently requires using RelationAdapter + RelationListUtil.marshal, which necessarily fixes SEARCH-444 too.

It would be technically possible to add separate adapter classes that only inject target-type while preserving the weird relation-list structure, but that would triple the amount of work for a temporary hack. So I'd propose we just fix both issues for areas and URLs during the schema change.

If we're just talking about adding target-type on its own, then while any change can be breaking, we've previously allowed adding additional properties without any prior announcement. I'm proposing we do the same here; but due to the above issue, only for events and works to start with.

Generally for JSON we have implemented this sort of improvement when it would be a breaking change by having both the old weird way and the new corrected way together in the response, haven't we? Is there a big reason this would not work here?

So, if we're talking about SEARCH-444, preserving the old weird way would require duplicating all relationships in the response.

@reosarevok

reosarevok commented Jan 14, 2026

Copy link
Copy Markdown
Member

Oh, I see. But normally the suggestion for breaking changes in the JSON stuff has been "with ws/3" - I'm not opposed to do so during the schema change, but then we need to be very clear about it and announce it very clearly in advance I expect, because waiting for ws/3 is why we duplicate all the other stuff now :) (and if we are willing to do that, I expect there's a bunch of other stuff we could fix at the same time...)

@mwiencek mwiencek changed the title SEARCH-751: Relationships are missing a target-type property SEARCH-751: Add the target-type property to relationships in event and work JSON Jan 14, 2026
…d work JSON

The `target-type` is also missing from area and URL relationships, but this
commit only affects event and work relationships. Extending the
`RelationAdapter` usage to areas and URLs would add the `target-type` but also
fix SEARCH-444, which is a larger breaking change.
@reosarevok

Copy link
Copy Markdown
Member

For future clarification, we agreed in a team meeting to release this now and fix the rest of the issues during the next schema change, as discussed above :)

@reosarevok reosarevok left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTMBDNT. I'd still appreciate an @yvanzo review as well.

@mwiencek
mwiencek requested a review from yvanzo January 22, 2026 21:46
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.

2 participants