Skip to content

Conversation

eldering
Copy link
Collaborator

Also explicitly mention that filtering only needs to be supported on properties listed in the access endpoint.

Adresses the "on a related note" in #193 (comment)

@eldering eldering requested review from niemela and johnbrvc July 14, 2025 23:11
@eldering
Copy link
Collaborator Author

This probably needs to be rebased after #193 is merged.

@deboer-tim
Copy link
Member

TBH I don't see the need for this change, to me ID and ID? have the same type. I.e. being optional doesn't change the type. I am not opposed to it if that's confusing someone else though.

The reference to /access actually confuses me though - isn't it obvious enough that you can't filter by something you can't see or doesn't exist? I'm not sure what we're trying to clarify here, but maybe there's a simpler way to say it.

Seeing this also reminded me of the 'except the id property'. I know it isn't being changed here, but I'm not sure if we were trying to save someone work to implement, and it isn't a great way to do it, but it is an odd restriction to make.

@eldering
Copy link
Collaborator Author

TBH I don't see the need for this change, to me ID and ID? have the same type. I.e. being optional doesn't change the type. I am not opposed to it if that's confusing someone else though.

The reference to /access actually confuses me though - isn't it obvious enough that you can't filter by something you can't see or doesn't exist? I'm not sure what we're trying to clarify here, but maybe there's a simpler way to say it.

I think I mostly agree, although this addition would make it explicit that if the property is in /access then you should be able to filter on it, even if in the endpoint itself the property is not explicitly visible because it has (almost) only null values. OTOH, then filtering is a bit pointless...

Seeing this also reminded me of the 'except the id property'. I know it isn't being changed here, but I'm not sure if we were trying to save someone work to implement, and it isn't a great way to do it, but it is an odd restriction to make.

Although it's an exception, I think this makes sense, since you can already "filter" on id by querying for that specific element instead of the whole collection endpoint.

All in all, I'm also fine with just dropping this PR.

niemela
niemela previously approved these changes Aug 25, 2025
Also explicitly mention that filtering only needs to be supported
on properties listed in the access endpoint.

Adresses the "on a related note" in
#193 (comment)
@eldering
Copy link
Collaborator Author

Rebased to fix a merge conflict with changes to the example of filtering on clarification recipient to sender.

@eldering eldering merged commit 9a87f1f into master Aug 29, 2025
@eldering eldering deleted the filter-nullable branch August 29, 2025 14:28
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