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

Enforce whitelist filtering #22

Merged
merged 19 commits into from
Jul 3, 2024
Merged

Conversation

lsfera
Copy link
Contributor

@lsfera lsfera commented Jun 22, 2024

closes #13
closes #10
Different specs for Pub/Sub
Allow message table customization

@lsfera lsfera marked this pull request as draft June 22, 2024 14:54
@lsfera lsfera marked this pull request as ready for review June 22, 2024 15:43
@lsfera lsfera marked this pull request as draft June 25, 2024 20:23
@lsfera lsfera marked this pull request as ready for review June 25, 2024 20:58
@lsfera lsfera changed the title Enforce provided whitelist at publication level Enforce whitelist filtering Jul 2, 2024
Copy link
Collaborator

@oskardudycz oskardudycz left a comment

Choose a reason for hiding this comment

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

@lsfera thank you for the next set of changes.

For the future, it'd be great if you could split the changes into a few Pull Requests, and discuss them before sending a big PR. Having such big scope of changes makes it hard to review and consider in terms of the general API. In this case, changes could be split e.g. into:

  • adding publication filter,
  • adding table descriptor,
  • ensuring ConfigureAwait is applied correctly,
  • etc.

I'd appreciate if you also consult bigger changes upfront.

I'm taking this PR as it is, as changes are good, and I want to review soon the current API and reshape it a bit in the follow-up set of changes.

@oskardudycz oskardudycz merged commit 1f4aa7a into event-driven-io:master Jul 3, 2024
1 check passed
@lsfera
Copy link
Contributor Author

lsfera commented Jul 3, 2024

@lsfera thank you for the next set of changes.

For the future, it'd be great if you could split the changes into a few Pull Requests, and discuss them before sending a big PR. Having such big scope of changes makes it hard to review and consider in terms of the general API. In this case, changes could be split e.g. into:

  • adding publication filter,
  • adding table descriptor,
  • ensuring ConfigureAwait is applied correctly,
  • etc.

I'd appreciate if you also consult bigger changes upfront.

I'm taking this PR as it is, as changes are good, and I want to review soon the current API and reshape it a bit in the follow-up set of changes.

Agree. I’m still in POC mode, as I want to add features to the project to verify how far we can go with it.
But definitely a more disciplined approach should help.
I’d like to have some open discussion to verify how to approach design choices.
E.g. #14 . We could opt for adding mime_type support along with bytea column for json too - having a unified approach despite the serialisation mechanism… but losing native postgresql advanced query support and compression optimised storage. Or maintain jsonb and bytea as separated use cases.
What are your thoughts?
Supporting object/bytes/text consuming is a thing. What about validating input data? Is this something we should care by design IMHO. Do you Agree?
Introducing DI. On ec2 I implemented background service worker by consumer type, wrapping them with back off retrying policy to deal with service bus/networking issues. Is this something we are interested here? Could a feature branch help on reasoning about it?

@oskardudycz
Copy link
Collaborator

@lsfera about different mime types, that sounds fair, but I'll need to do more research on e.g. how others are doing it (e.g. Debezium). EventStoreDB stores mime/type and always stores bytes. Then you can select your serialiser based on that.

Definitely what I'd like to have is not enforce deserialisation of messages. This would, e.g. enable just forwarding messages as they are to other messaging systems or even streaming them through, e.g. web sockets.

For that probably there'll be need to split:

  • raw async enumerable,
  • on top of that async enumerable with deserialised messages,
  • probably some consumer abstractions for single and batched handling.

Maybe others, but that would enable different use cases and even other tools like Brighter, MassTransit to use Blümchen internally and also regular applications for business needs. I'll need to sleep on that. Definitely, if I have some draft, I'll tag you in the PR.

I think that we should not be doing any validation or manipulation of the data we're getting from the users. Even from the legal audit perspective it's better for the user to be sure that what they store is what they get in the database.

Introducing DI. On ec2 I implemented background service worker by consumer type, wrapping them with back off retrying policy to deal with service bus/networking issues. Is this something we are interested here? Could a feature branch help on reasoning about it?

Yup, this would definitely help people having the capability to just do AddBlumchen and configure the default options.

Also, later on I'd like to add WebHooks built-in support. That could be useful for doing serverless running Blumchen service e.g. on Fargate, that'd just forward events through HTTP to SQS or other services.

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.

Add support for Publication filters Mark JetBrains.Annotations as development dependency
2 participants