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 for using cache pools with multiple areas #2232

Merged
merged 12 commits into from Mar 6, 2024

Conversation

Brajk19
Copy link
Contributor

@Brajk19 Brajk19 commented Mar 4, 2024

#2225 enabled usage of cache pools.
I noticed a problem when using this with multiple areas.

If i have configuration with area1 and area2, depending on which ApiDocGenerator is called first, its specification will be cached and all following generators (for any area) will return that cached response because same item id is used.

Defining global cache (for all areas) is deprecated and all cache config should be area specific.

nelmio_api_doc:
    areas:
        default:
            cache:
                pool: app.cache
                item_id: docs
        area2:
            cache:
                pool: app.cache
                item_id: docs2
    ...

@Brajk19 Brajk19 changed the title possible fix for cache pools and different areas Fix for using cache pools with multiple areas Mar 4, 2024
@DjordyKoert
Copy link
Collaborator

I'm not sure what would be best fix for this. I've thought about moving cache.pool and cache.item_id config under areas but that would break BC.

Let's deprecate the cache.pool and cache.item_id and move the cache config to areas.

Other idea is that area is automatically appended to cache key so if we have config like this:

I don't think we should do this.

@Brajk19
Copy link
Contributor Author

Brajk19 commented Mar 4, 2024

I'm not sure what would be best fix for this. I've thought about moving cache.pool and cache.item_id config under areas but that would break BC.

Let's deprecate the cache.pool and cache.item_id and move the cache config to areas.

Other idea is that area is automatically appended to cache key so if we have config like this:

I don't think we should do this.

Alright. I'll move it to areas 👍

@Brajk19 Brajk19 marked this pull request as draft March 4, 2024 16:05
@Brajk19 Brajk19 marked this pull request as ready for review March 5, 2024 11:46
@DjordyKoert
Copy link
Collaborator

I'm not sure what would be best fix for this. I've thought about moving cache.pool and cache.item_id config under areas but that would break BC.

Let's deprecate the cache.pool and cache.item_id and move the cache config to areas.

Looking at the per area implementation that you made I think it won't be necessary to deprecate the old cache configuration.
It doesn't hurt to automatically append the area to the cache.item_id if this happens

Other idea is that area is automatically appended to cache key so if we have config like this:
I don't think we should do this.

I also fully take this back (my bad 😅) the old behaviour (before the cache option was implemented #2225) was already broken if someone if someone did the following:

  • Manually define the cache args for the ApiDocGenerator service(s)
  • Forget to set an arg for the cacheItemId
  • Use multiple areas in the same cache pool

Doing above these steps would also result in the cache to be shared between areas.

@Brajk19
Copy link
Contributor Author

Brajk19 commented Mar 5, 2024

How about this:

  • we leave old configuration as is, do not deprecate it
  • if cache is defined globally it will be used for every area
  • if it's also defined for area, that that config will be used

so basically global config is used as fallback if area-specific config is not defined

i could have something like this

nelmio_api_doc:
   cache:
       pool: app.cache
   areas:
       default:
           cache:
               item_id: docs
       area2:
           cache: 
               item_id: docs2
   ...

same pool, different item ids

@Brajk19
Copy link
Contributor Author

Brajk19 commented Mar 5, 2024

i'm just not sure how to fit auto appending area to cache key into this logic
should we even do that?

@DjordyKoert
Copy link
Collaborator

How about this:

  • we leave old configuration as is, do not deprecate it
  • if cache is defined globally it will be used for every area
  • if it's also defined for area, that that config will be used

so basically global config is used as fallback if area-specific config is not defined

i could have something like this

nelmio_api_doc:
   cache:
       pool: app.cache
   areas:
       default:
           cache:
               item_id: docs
       area2:
           cache: 
               item_id: docs2
   ...

same pool, different item ids

Looks good to me! Below are some more configuration combinations I can think of which seem reasonable to allow:

# Uses default `item_id` (openapi_docs) and appends the area => `openapi_docs.default`,  `openapi_docs.area1`
nelmio_api_doc:
   cache:
       pool: app.cache
   areas:
       default:
   ...
# Only apply cache on a single area
nelmio_api_doc:
   areas:
       default:
           ...
       area2:
           cache: 
               pool: app.cache
               item_id: docs
   ...
# Sharing cache between areas
nelmio_api_doc:
   cache:
       pool: app.cache
   areas:
       default: # Uses cache from global
           ...
       area2:
           cache: 
               item_id: shared_cache_id
       shared_area:
           cache: 
               item_id: shared_cache_id
   ...

@DjordyKoert
Copy link
Collaborator

i'm just not sure how to fit auto appending area to cache key into this logic should we even do that?

I think we should. I can also create something for this really quick

@Brajk19
Copy link
Contributor Author

Brajk19 commented Mar 5, 2024

i'm just not sure how to fit auto appending area to cache key into this logic should we even do that?

I think we should. I can also create something for this really quick

c8e7c8f
i added change which removes deprecation and uses global config as fallback if area-specific config is not defined
feel free to add that auto appending of area how you envisioned it 😄

@DjordyKoert DjordyKoert merged commit 32537bb into nelmio:master Mar 6, 2024
12 checks passed
@DjordyKoert
Copy link
Collaborator

Thank you @Brajk19!

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.

None yet

3 participants