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

Bugfix: Fixed Configuration Model for Redirect Manager after change in "redirect" resource as "sling:Folder" #3479

Conversation

vabs95
Copy link
Contributor

@vabs95 vabs95 commented Nov 24, 2024

Issue: Regression

Cause:

The resource type for redirects has been changed to "sling:Folder". However, the JCR query used to display the list of configurations has not been updated and currently only retrieves nodes of type "nt:unstructured".

Fix:

Refactor Configurations Model to read the configs directly without query.

Regression via #3399

@vabs95
Copy link
Contributor Author

vabs95 commented Nov 24, 2024

FYI @kwin and @YegorKozlov

@vabs95 vabs95 force-pushed the bugfix/redirect-manager-jcrquery-fix branch from 490b9c8 to 979a298 Compare November 24, 2024 12:49
Copy link
Contributor

@YegorKozlov YegorKozlov left a comment

Choose a reason for hiding this comment

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

@vabs95 a good catch!

I'd say we should rewrite the code further and get rid of the jcr query at all. We can simply iterate over /conf/*, something like:

    public Collection<RedirectConfiguration> getConfigurations() {
        List<RedirectConfiguration> lst = new ArrayList<>();
        String bucketName = redirectFilter == null ? RedirectFilter.DEFAULT_CONFIG_BUCKET : redirectFilter.getBucket();
        String configName = redirectFilter == null ? RedirectFilter.DEFAULT_CONFIG_NAME : redirectFilter.getConfigName();
        Resource conf = request.getResourceResolver().getResource("/conf");
        for (Resource confRoot : conf.getChildren()) {
            String storageSuffix = bucketName + "/" + configName;
            Resource res = confRoot.getChild(storageSuffix);
            if (res != null && res.isResourceType(REDIRECTS_RESOURCE_TYPE)) {
                RedirectConfiguration cfg = new RedirectConfiguration(res, storageSuffix);
                lst.add(cfg);
            }
        }
        lst.sort(Comparator.comparing(RedirectConfiguration::getName));
        return lst;
    }

It would be also great to update the tests. There are tests for CreateRedirectConfigurationServlet, e.g.
https://github.com/Adobe-Consulting-Services/acs-aem-commons/blob/213279b88757a3798893397f09ec74e39f16a1b3/bundle/src/test/java/com/adobe/acs/commons/redirects/servlets/CreateRedirectConfigurationServletTest.java#L60C1-L61C1

but the way they read the created resources is not aligned with the code.
instead of reading the resource directly:

        Resource cfg = context.resourceResolver().getResource("/conf/global/settings/redirects");

the test should read via the model:

        Configurations confModel = context.request().adaptTo(Configurations.class);
        Collection<RedirectConfiguration> configurations = confModel.getConfigurations();
        RedirectConfiguration cfg = configurations.iterator().next();
        assertEquals("/conf/global/settings/redirects", cfg.getPath());

this way the test will cover both the create and read parts.

@vabs95 vabs95 force-pushed the bugfix/redirect-manager-jcrquery-fix branch from 979a298 to 0bda15b Compare November 24, 2024 18:41
@vabs95
Copy link
Contributor Author

vabs95 commented Nov 24, 2024

@YegorKozlov Changes done as requested.

@vabs95 vabs95 requested a review from YegorKozlov November 24, 2024 18:44
@vabs95 vabs95 force-pushed the bugfix/redirect-manager-jcrquery-fix branch from 0bda15b to 5644814 Compare November 24, 2024 18:50
@vabs95 vabs95 changed the title Bugfix: Fixed JCR Query for Redirect Manager after change in "redirect" resource as "sling:Folder" Bugfix: Fixed Configuration Model for Redirect Manager after change in "redirect" resource as "sling:Folder" Nov 24, 2024
@davidjgonzalez davidjgonzalez merged commit 06461dd into Adobe-Consulting-Services:master Dec 5, 2024
19 checks passed
@davidjgonzalez davidjgonzalez added this to the 6.9.8 milestone Dec 5, 2024
@davidjgonzalez
Copy link
Contributor

Neither here nor there, but if you do write a query, and Oak determines that the faster way to collect the results is to just traverse the tree, it will traverse the tree. The overhead of the query is negligible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants