Skip to content

[ul] Implement generic Extended Negotiation#734

Open
pgimeno4d wants to merge 1 commit intoEnet4:masterfrom
pgimeno4d:implement-extended-negotiation
Open

[ul] Implement generic Extended Negotiation#734
pgimeno4d wants to merge 1 commit intoEnet4:masterfrom
pgimeno4d:implement-extended-negotiation

Conversation

@pgimeno4d
Copy link
Copy Markdown
Contributor

@pgimeno4d pgimeno4d commented Feb 3, 2026

Add a Negotiation trait which contains, for the moment, the extended_negotiation method. In future it can also help implement Role Selection negotiation (#731) and possibly other currently unimplemented features like Common Extended Negotiation. Note I'm also interested in #731 and will probably submit a patch if this one is merged, as it will depend on said trait.

This is a case where the client and the server differ substantially. The Negotiation trait is only for the server; the client just adds pairs SOP Class / Byte array. After the association succeeds, they both have the same set of pairs, which they can look up to determine which extended options are active, through the new method extended_negotiation_for(sop_class_uid: &str) or by walking the user_variables().

Everything is quite low-level, to cover for all current and hopefully future versions of the standard. It's easy to send invalid bytes, for instance.

I'm not quite sure if this is a breaking change. It did not break anything in our application, which is a server. The addition of a new generic concerned me a bit.

Edit: Forgot to mention, this obviously depends on #727, otherwise the bytestream is not correct.

@pgimeno4d pgimeno4d force-pushed the implement-extended-negotiation branch from 9cd9c0a to c9ad719 Compare February 9, 2026 06:58
@pgimeno4d
Copy link
Copy Markdown
Contributor Author

pgimeno4d commented Feb 9, 2026

Rebased on top of #727 so that it can be properly tested with that change applied.

@Enet4 Enet4 added A-lib Area: library C-ul Crate: dicom-ul labels Feb 10, 2026
@Enet4 Enet4 added the breaking change Hint that this may require a major version bump on release label Mar 3, 2026
@Enet4 Enet4 added this to the DICOM-rs 0.10 milestone Mar 3, 2026
@Enet4
Copy link
Copy Markdown
Owner

Enet4 commented Mar 3, 2026

Everything is quite low-level, to cover for all current and hopefully future versions of the standard. It's easy to send invalid bytes, for instance.

I guess it is fine for now. I suspect doing more than this would require the inclusion of dicom_object in dicom_ul, which is something I have intentionally avoided to reduce the complexity of this part of the API.

I'm not quite sure if this is a breaking change. It did not break anything in our application, which is a server. The addition of a new generic concerned me a bit.

That extra type parameter in ServerAssociationOptions, plus the additional method in the Association trait, makes it a breaking change. It is fine, it only means that we can bring this in for 0.10.0, and in any case the project is due for another release not long after 0.9.1.

@Enet4 Enet4 self-requested a review March 4, 2026 08:38
@stefan-hubert-bl
Copy link
Copy Markdown

Hi @Enet4 and @pgimeno4d: Revisiting this while monitoring status for the other issue #731, which depends on this. Do I understand correctly, that you're waiting to proceed here because this being a breaking change? ... and you don't want to merge this until you're preparing the next version 0.10 which allows breaking changes? What merge policy are you following: do you have a dedicated separate branch for breaking changes? Or do you wait with merging those until you think time has come for breaking changes on master? Considering this, when do you think is a good time to merge this? (I observed, you did one or two releases (which allowed breaking changes) per year in the past... Thank you!

@Enet4
Copy link
Copy Markdown
Owner

Enet4 commented Mar 24, 2026

Thank you for your message @stefan-hubert-bl.

Revisiting this while monitoring status for the other issue #731, which depends on this. Do I understand correctly, that you're waiting to proceed here because this being a breaking change? ... and you don't want to merge this until you're preparing the next version 0.10 which allows breaking changes? What merge policy are you following: do you have a dedicated separate branch for breaking changes? Or do you wait with merging those until you think time has come for breaking changes on master? Considering this, when do you think is a good time to merge this? (I observed, you did one or two releases (which allowed breaking changes) per year in the past... Thank you!

The current merging policy would be more of the latter: when there are enough non-breaking contributions to justify a patch release, I merge those and release. There is no fixed timeline, since it has mostly depended on my own availability to work the pull requests.

In any case, what I wish to do next is wrap up 0.9.1 by merging #752 and initiating the release process, creating and updating all crates affected. Then the next version will definitely be 0.10.0 and should likely include this PR, so I will do further testing and reviewing at that time to make it happen.

@pgimeno4d pgimeno4d force-pushed the implement-extended-negotiation branch from c9ad719 to 0c5de4e Compare March 25, 2026 09:08
@pgimeno4d
Copy link
Copy Markdown
Contributor Author

Rebased, squashed, and some very minor wording changes applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-lib Area: library breaking change Hint that this may require a major version bump on release C-ul Crate: dicom-ul

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants