-
Notifications
You must be signed in to change notification settings - Fork 33
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
ImageflowMiddlewareOptions.AddWatermark() - Removal #13
Comments
It's not currently thread-safe to modify ImageflowMiddlewareOptions during requests. It's not designed to be used that way. I'll think on how to enable your scenario in a safe manner and get back to you. |
Thanks @lilith. And just to note, I have no need for these dynamically generated watermarks to live throughout the lifetime of the application, I just need them created and used for the particular image request at hand. Potentially, every image coming down the pipeline could have a different watermark configuration. So if there was a way during AddRewriteHandler to add a watermark config for JUST this particular request, this would not lose our thread safety. |
I've released 0.3.6 with AddWatermarkingHandler() support. This allows you to manipulate the list of applied watermarks for just that request, and gives you access to the path, query, and http context. |
Hi Lilith, I tested this out and it seems to be working well for my use cases. Also (kind of) an added benefit that I don't need to actually modify the query string to apply a "watermark=something" since we're modifying the collection itself. This is also somewhat of a downside since you made it to where the WatermarkingEventArgs.Query property is read-only AND the handler is processed too late in the chain anyway for the query options to be applied. So I'll still need to use one of the other rewrite rules to apply any query changes. Since I need to do some external processing (DB queries) to find out the conditional watermarking AND conditional query parameters that need to be applied (on a per-image basis), then potentially I need to do the same processing TWICE. However, luckily I stumbled upon HttpContext.Items that will allow me to share that same conditional logic across the single request processing chain. So really not a big deal since the order of execution is predictable for a single request, but kind of a hidden gem if you're unfamiliar with inner workings of ASP.NET processing chain. Maybe wouldn't be a bad idea to put into one of your examples to show how you can share information across all four handlers for a SINGLE request: AddPreRewriteAuthorizationHandler, AddRewriteHandler, AddPostRewriteAuthorizationHandler, AddWatermarkingHandler. Thank you for implementing something so quickly. It is much appreciated! :) |
Glad it is working for you. I couldn't let the querystring be modifiable because I needed the final &watermark value to populate the initial list of watermarks. Good idea on documenting HttpContext.Items |
@lilith Found a bug with the process of handling custom/dynamic watermarks in this way. Watermarks added via "AddWatermarkingHandler" will ONLY be added if at least ONE query string is added. But if none are added, then the custom watermarks are NOT processed. I know part of the issue boils down to the ImageJobInfo.NeedsCaching() method returning false in these cases, however, I think the problem is further stretched wherever ImageJobInfo.HasParams is being used. The solution might be as simple as when you're looping over appliedWatermarks in the ImageJobInfo constructor, set HasParams to true, but I'm not really sure what else this might affect. Another possible solution might be to move that handler logic above where HasParams is set, and then during the looping process, add a custom query string "appliedWatermarks=true" or something. I suppose for the time being I can just set something in PathHelpers.querystringKeys that will have virtually no effect on the end result, like autorotate or format... |
@lilith just following up to see if you had any plans to resolve the issue I outlined above, or should I open a new issue? |
@gerneio Sorry for reopening this so many years later; I'm afraid my github notifications get really swamped and I end up having to manually check issues. This area is being completely re-done, but at the same time I'm not sure running handlers for every single request in the app is a good idea. At minimum this should be documented, though. |
I need to dynamically generate watermark configurations on the fly, conditionally depending on the current image being requested. I am using the "AddRewriteHandler" to implement my conditional logic, which might involve some DB calls. While I can ADD one without an issue, I can not remove that watermark configuration or replace it, which may be necessary in case the dynamic conditions change on the next call (in addition to cleanup). In this project's current state, is there a way to accomplish this? It seems that the ImageflowMiddlewareOptions.NamedWatermarks is a IReadOnlyCollection, and therefore can't be modified directly. While I can filter down to the previously added NamedWatermark object, Now, even though WatermarkOptions is GET only, I can use its methods to modify the state of the object, but even then I wouldn't know what all was set previously and what needs to be reset.
For me, the simplest solution would be to give us the ability to clean up these NamedWatermarks so that we can just remove and add as necessary.
The text was updated successfully, but these errors were encountered: