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

Resolve Issue 1386, Add rpc_prefix #1484

Merged
merged 13 commits into from
Jun 17, 2024
Merged

Resolve Issue 1386, Add rpc_prefix #1484

merged 13 commits into from
Jun 17, 2024

Conversation

aKardasz
Copy link
Contributor

@aKardasz aKardasz commented May 29, 2024

Description

Adds nuid from nats broker to generation unique ids.
Adds rpc_prefix, to allow support for nats streams inbox_. functionality via permissions, for consistency added the same rpc_prefix to the redis implementation.

Fixes #1386

Type of change

Please delete options that are not relevant.

  • Documentation (typos, code examples, or any documentation updates)
  • New feature (a non-breaking change that adds functionality)

Checklist

  • My code adheres to the style guidelines of this project (scripts/lint.sh shows no errors)
  • I have conducted a self-review of my own code
  • My changes do not generate any new warnings
  • I have added tests to validate the effectiveness of my fix or the functionality of my new feature
  • Both new and existing unit tests pass successfully on my local environment by running scripts/test-cov.sh
  • I have ensured that static analysis tests are passing by running scripts/static-analysis.sh

@aKardasz
Copy link
Contributor Author

aKardasz commented May 29, 2024

Statis analysis error, not sure how you guys would like to go about this issue in static analysis.


Test results:
>> Issue: [B311:blacklist] Standard pseudo-random generators are not suitable for security/cryptographic purposes.
   Severity: Low   Confidence: High
   CWE: CWE-330 (https://cwe.mitre.org/data/definitions/330.html)
   More Info: https://bandit.readthedocs.io/en/1.7.8/blacklists/blacklist_calls.html#b311-random
   Location: faststream/utils/nuid.py:40:22
39          def __init__(self) -> None:
40              self._prand = Random(randbelow(max_int))
41              self._seq = self._prand.randint(0, MAX_SEQ)

--------------------------------------------------

Code scanned:
        Total lines of code: 34801
        Total lines skipped (#nosec): 0

Run metrics:
        Total issues (by severity):
                Undefined: 0
                Low: 1
                Medium: 0
                High: 0
        Total issues (by confidence):
                Undefined: 0
                Low: 0
                Medium: 0
                High: 1
Files skipped (0):

@Lancetnik
Copy link
Member

We can no worry about this warning due we doesn't use random for cryptography
You can just suppress it

Copy link
Member

@Lancetnik Lancetnik left a comment

Choose a reason for hiding this comment

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

I don't like an idea to add an extra option if nats-py already has the mechanism we can reuse

@aKardasz
Copy link
Contributor Author

aKardasz commented Jun 4, 2024

@Lancetnik with the removal of rpc_prefix, realistically that would mean no new tests are needed, would that be a safe assumption since the inbox update would just fall under regular tests.

Also, having a hard time running the tests in github workspaces, maybe we can do a setup guide for others or like a make file what are your thoughts?

@Lancetnik
Copy link
Member

@aKardasz I think, we should update CONTRIBUTING file to allow users to contribute easily

About testing - I think we should check NatsBroker(inbox_prefix=...) correct behavior

Can you please add any test for it and then we can merge the branch?

@davorrunje
Copy link
Collaborator

Also, having a hard time running the tests in github workspaces, maybe we can do a setup guide for others or like a make file what are your thoughts?

We could create devcontainers with the complete setup for testing particular MQ. That would probably be the best solution.

@aKardasz
Copy link
Contributor Author

@aKardasz I think, we should update CONTRIBUTING file to allow users to contribute easily

About testing - I think we should check NatsBroker(inbox_prefix=...) correct behavior

Can you please add any test for it and then we can merge the branch?

Let me know if the added test is sufficient.

@aKardasz aKardasz marked this pull request as ready for review June 13, 2024 16:19
@Lancetnik Lancetnik enabled auto-merge June 17, 2024 19:05
@Lancetnik Lancetnik added this pull request to the merge queue Jun 17, 2024
Merged via the queue into airtai:main with commit 1e87e76 Jun 17, 2024
29 checks passed
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.

Feature: more stronger unique RPC subject names
3 participants