Skip to content

Comments

prepare PR 2#2

Open
devslovecoffee wants to merge 1 commit intopr-1from
pr-2
Open

prepare PR 2#2
devslovecoffee wants to merge 1 commit intopr-1from
pr-2

Conversation

@devslovecoffee
Copy link
Collaborator

@devslovecoffee devslovecoffee commented Aug 1, 2024

Description by Cal

PR Description

This PR updates various dependencies in the project, including version upgrades for rustls, clap, bstr, and others. It also introduces a new EntityCacheStorage enum and modifies the EntityCachingConfig structure to include a storage option.

Key Issues

None

Files Changed

File: /Cargo.lock Updated multiple dependencies to their latest versions.
File: /engine/crates/engine-config-builder/src/from_sdl_config.rs Modified pattern matching to include additional fields in `EntityCachingConfig`.
File: /engine/crates/engine-v2/src/operation/metrics.rs Added a new field `used_fields` to the metrics attributes.
File: /engine/crates/engine/response/src/lib.rs Updated the `GraphqlOperationAnalyticsAttributes` struct to include `used_fields`.
File: /engine/crates/engine/src/schema.rs Modified the handling of operation analytics attributes to include `used_fields`.
File: /engine/crates/gateway-core/src/cache/partial.rs Updated the response creation to include `used_fields`.
File: /engine/crates/parser-sdl/src/federation.rs Introduced a new enum `EntityCacheStorage` and updated `EntityCachingConfig`.
File: /engine/crates/runtime-local/src/lib.rs Added Redis module for rate limiting.
File: /engine/crates/runtime-local/src/rate_limiting/redis.rs Implemented Redis rate limiting functionality.
File: /gateway/crates/config/src/entity_caching.rs Updated `EntityCachingConfig` to include Redis configuration options.
# Description

Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.

Type of change

  • 💔 Breaking
  • 🚀 Feature
  • 🐛 Fix
  • 🛠️ Tooling
  • 🔨 Refactoring
  • 🧪 Test
  • 📦 Dependency
  • 📖 Requires documentation update

) -> EntityCacheStorage {
match storage {
gateway_config::EntityCachingStorage::Memory => EntityCacheStorage::Memory,
gateway_config::EntityCachingStorage::Redis => EntityCacheStorage::Redis(redis.unwrap_or_default().into()),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🐛 Bug
The function entity_cache_storage does not handle the case where redis is None for the Redis storage variant. This could lead to a panic if redis.unwrap_or_default() is called when redis is None. You should handle this case more gracefully.

Suggested change
gateway_config::EntityCachingStorage::Redis => EntityCacheStorage::Redis(redis.unwrap_or_default().into()),
EntityCacheStorage::Redis(redis.unwrap_or_else(|| gateway_config::EntityCachingRedisConfig::default().into()))

fn new_pool(url: &str, tls_config: Option<RedisTlsConfig<'_>>) -> anyhow::Result<Pool> {
let tls_config = match tls_config {
Some(tls) => {
let client_tls = match tls.cert.zip(tls.key) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggestion
In the new_pool function, consider handling the case where tls.cert or tls.key is None more explicitly, to avoid potential panics when calling zip on None values.

});

let pool = redis_factory
.pool(config.redis.url, tls)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggestion
Ensure that the error handling for the Redis connection pool creation is robust and provides meaningful feedback.

[[package]]
name = "rustls"
version = "0.23.10"
version = "0.23.12"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggestion
Check if the new version of rustls includes any security patches or important updates that need to be addressed in your application.

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.

1 participant