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 a couple of semi-edge-cases #199

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

lanfeust69
Copy link
Contributor

These could result in not binding a service at all, when only one method is at fault, or throwing unexpectedly on the client side.

Copy link
Member

@mgravell mgravell left a comment

Choose a reason for hiding this comment

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

  • IDisposable - makes sense, nice
  • CanSerialize - should probably fix in protobuf-net too, but this works
  • but: what's the scenario for delegates? in what scenario would we expect/need them to work?

Looking good, though; liking where this is going

Recommendation: nameof(IDisposable.Dispose) instead of "Dispose"

@lanfeust69
Copy link
Contributor Author

  • but: what's the scenario for delegates? in what scenario would we expect/need them to work?

No scenario, the code was from before the use of the TypeCategory.Invalid, and defaulting everything to TypeCategory.Data.

The real (small) issue is that we happen to have a Wcf operation that did have an Action parameter (no idea what this actually did), which was "accepted" (with no argument at all because of the TypeCategory.None) as if nothing was wrong, instead of being rejected with a warning, like with any other type not reported as serializable by the marshallerCache.

Recommendation: nameof(IDisposable.Dispose) instead of "Dispose"
Yeah, I added at the last moment since it's not strictly necessary (there is nothing else on IDisposable), but seemed clearer, and I was a bit fast, I'll change that...

There is at least one situation where the underlying protobuf TypeModel
throws a NotSupportedException : when asking about an array of arrays.
While this seems doubtful (it should simply return false), it is easy
enough to prevent this from trashing the whole service.
No longer any reason to handle delegates specifically : it will be done
naturally by the marshallerCache (and who knows, maybe it *can*
serialize some delegates !).
Moreover, this could misinterpret a signature as valid without parameter,
with somewhat weird consequences.
@mgravell mgravell merged commit 4781096 into protobuf-net:main Feb 2, 2023
@mgravell
Copy link
Member

mgravell commented Feb 2, 2023

this was long overdue - entirely my fault; merged with thanks - should be in 1.1

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