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

Interest in new search filters for nested tags, folders, maybe other nestable things #1058

Open
mfyobst opened this issue Dec 11, 2023 · 14 comments

Comments

@mfyobst
Copy link

mfyobst commented Dec 11, 2023

Hello,

In getting familiar with aem, asc and java (coming most recently from c#) I've built a couple search filters for nested "sling:Folder" and "cq:Tag". Both override a couple methods from a base AbsrtactNestedPredicate containing most of the code. It should be easy to override the base as needed for other nested resource types.

The ui is also mostly shared using sling:resourceSuperType with just 60 lines vanilla js/css. Parent/child checkbox state is propagated automatically, though I'm considering making that optional in _cq_dialog for some edge cases.

The screenshots are probably enough to gauge any interest and whether I should pursue a pull request.
Let me know if there are any questions,
Thanks

filters
tags
folders

@davidjgonzalez
Copy link
Contributor

@mfyobst these looks very cool! Would def be open to a PR if its not too much trouble -- or if you had the code in an accessible repo to look at it there for compat.

One thing to caution about the folder predicate (which i was bitten by myself) is that the Search Results component (which Java-side is ~represented by the PagePredicateImpl) also has path restrictions in it - and that config is expected to be the "lowest level" path restriction for the search -- so any of the Folder criteria would need to 1) validate its under any of those paths and 2) ensure they are not OR'd together .. else any of the "sub-path" restrictions will be supeseded by the lower-level rooth paths.

niether here nor there - but what does "Children merged" mean/do? That ones not obvious to me.

@mfyobst
Copy link
Author

mfyobst commented Dec 12, 2023

Hello David,
Great, I just need to figure out business-wise if I can publish open-source and will let you know. The company I work for is in a "strategic partnership" with Adobe so it should be ok.

Thanks for the details on path restrictions, and I think explains some inconsistent search results that confused me, for example:
(/content/dam/asset-share-commons/en/site OR /content/dam/asset-share-commons/en/public/pictures) returns all child assets
(/content/dam/asset-share-commons/en/site OR /content/dam/asset-share-commons/en/public) returns empty

So if I'm getting it, OR'd path restrictions bubble up: ("a" OR "a/b/c") "a/b/c supersedes "a"; is "a/z" unaffected?
I'm of half-a-mind to just ignore as expected (but arbitrary) behavior by the framework. I try to avoid fighting frameworks, especially around security, especially when a noob.
Maybe just adding a "Allow restricted folders" checkbox: (default unchecked) invalidate a root path if it contains any restricted descendants of resourceType.

Regarding "Children Merged" checkbox, the info tip is "Merge top-level children of all root paths" which probably helps only a little. In other words, the immediate-children of all root paths are combined and listed top-level in the ui. So, "/content/dam/asset-share-commons/en/public" would display "Documents", "Media", "Pictures" top-level, instead of just "Public" which would be omitted.

Thanks for the feedback and I hope to push somewhere soon.
Max

@davidjgonzalez
Copy link
Contributor

For path restrictions i meant more:

If the the Search Results component [1] sets the base path restrictions (ex. /content/dam/asset-share-commons/en/public) that can be allowed to surface in search. So if in your component you somehow were able to set a folder selection to /content/dam/foo (which is not under /content/dam/asset-share-commons/en/public) the restriction of /content/dam/foo should get discarded. This should be taken care of if youre using the OOTB path predicate though. As part of the QuerySearchProviderImpl, .. there is some special case handling [2] that looks for path predicates coming in from query params, and makes sure anything thats passed in falls under one of the Search Results [1] paths.
If the query parameter path is allowed (its same or beneath an authored Search Resutls path; we dont want random ppl to be able to specify any path in AEM Assets and start querying it :)) - then it marks the Search Results path to be excluded from being added to the query (since we know the users picks an allowed sub-path(s) to search on -- and the PagePredicatImpl [3] skips adding the SearchResults component's path to the overall query.

Anyhow - its a bit convoluted/tricky ... not sure i have the most elegant solution for it - but i think youre good as long as you

  1. Communicate to authors configuring the Paths component that only sub-paths of a Search Results path will work
  2. Use the OOTB querybuilder path predicate

[1]
2023-12-12 at 9 05 PM

[2] https://github.com/adobe/asset-share-commons/blob/develop/core/src/main/java/com/adobe/aem/commons/assetshare/search/providers/impl/QuerySearchProviderImpl.java#L172-L220

[3] https://github.com/adobe/asset-share-commons/blob/develop/core/src/main/java/com/adobe/aem/commons/assetshare/components/predicates/impl/PagePredicateImpl.java#L195-L199

@davidjgonzalez
Copy link
Contributor

Gotcha on the Merge children - that makes sense ... you can create an artificial set of "root" folders, pulling from various levels of the DAM. I can definitely see this being useful.

@mfyobst
Copy link
Author

mfyobst commented Jan 3, 2024

Hello David, I finally got code pushed to: https://github.com/mfyobst/slop
Let me know when you get a chance to have a look, it definitely needs some code review for java/aem newb mistakes. There is no testing yet, but I'm looking through core\src\test\java\com\adobe\aem\commons\assetshare atm trying to figure out what's expected.

@mfyobst
Copy link
Author

mfyobst commented Jan 3, 2024

Forgot to mention, built from release 3.5.0. I'm using windows/vscode and anything newer fails with a long list of bnd-baseline version problems that has me stumped for now.

@davidjgonzalez
Copy link
Contributor

Let me take a look at it tomorrow or Friday. The bnd baseline can be nitpicky

@davidjgonzalez
Copy link
Contributor

@mfyobst It looks like your github has cherrypicked code - rather than the whole project; so what you ahve isnt a valid Maven project and/or would be merge-able (wrt to Git) into this project.

Typically, youd want to:

  1. Fork this repo
  2. Create a new working branch from this repo's develop branch
  3. Put your changes in that working branch
  4. Commit your changes
  5. Push the working branch to your fork
  6. PR that working branch on your fork to the this repo's develop branch

@mfyobst
Copy link
Author

mfyobst commented Jan 4, 2024

Ok, that was just a git diff > diff.patch/git apply diff.patch to an empty repo but I just forked and pushed to https://github.com/mfyobst/asset-share-commons

Note I'm still working on baseline issues:
[ERROR] Failed to execute goal biz.aQute.bnd:bnd-baseline-maven-plugin:6.3.1:baseline (baseline) on project assetshare.core: An error occurred while calculating the baseline: Baseline problems detected. See the report in C:\work\mf-asc\core\target\baseline\assetshare.core-3.8.1-SNAPSHOT.txt.
[ERROR] ===============================================================
[ERROR] Name Type Delta New Old Suggest
[ERROR] * com.adobe.aem.commons.assetshare.core BUNDLE MAJOR 3.8.1.20240104162143091 3.8.0 3.9.0
[ERROR] MAJOR BUNDLE assetshare.core-3.8.1-SNAPSHOT
[ERROR] MINOR API
[ERROR] MINOR PACKAGE com.adobe.aem.commons.assetshare.components.predicates
[ERROR] ADDED CLASS com.adobe.aem.commons.assetshare.components.predicates.AbstractNestedPredicate
[ERROR] ADDED ACCESS abstract
[ERROR] ADDED EXTENDS com.adobe.aem.commons.assetshare.components.predicates.AbstractPredicate
[ERROR] ADDED IMPLEMENTS com.adobe.aem.commons.assetshare.components.Component
[ERROR] ADDED IMPLEMENTS com.adobe.aem.commons.assetshare.components.predicates.NestedPredicate
...

I've tried deleting .m2 cache folder and skipping the test with -Darguments="-Dbnd.baseline.skip=true" but no luck so far.

@davidjgonzalez
Copy link
Contributor

ahh sorry - this is saying your change requires a bundle version bump to 3.9.0 -- and since we pin all projects to the same version for ASC, youll need to bump them all:

so run this from the root of your project:

$ mvn versions:set -DnewVersion=3.9.0-SNAPSHOT

This will touch and update all pom.xml files in the project to 3.9.0-SNAPSHOT and bnd-baseline should be happy ...

You may need to also update a package-info.java in com.adobe.aem.commons.assetshare.components.predicates but bnd-baseline will output what the correct version there is.

@mfyobst
Copy link
Author

mfyobst commented Jan 4, 2024

Thanks, that worked. Also, got skip working by updating core/pom.xml:

<plugin>
    <groupId>biz.aQute.bnd</groupId>
    <artifactId>bnd-baseline-maven-plugin</artifactId>
    <configuration>
        <failOnMissing>false</failOnMissing>
        <skip>true</skip>
    </configuration>
</plugin>

@davidjgonzalez
Copy link
Contributor

Just to be clear we don't want to skip bnd baseline recommendations - they're important ensure osgi import/exports are correct.

@mfyobst
Copy link
Author

mfyobst commented Jan 5, 2024

Got it, won't skip baseline testing. Should modified pom.xmls/pakage-infos be pushed or are they just for local dev?
Fyi, for windows/vscode/powershell I needed quotes: mvn versions:set -DnewVersion="3.9.0-SNAPSHOT"

@davidjgonzalez
Copy link
Contributor

both the POMs and package-info should be committed to Git -- these are used to build and version the artifacts and bundle exports when installed into AEM, so these version notations are part of the code base.

Ah - good to know about windows, im on macOS ... as long as the POMs are all getting updated to 3.9.0-SNAPSHOT version it's all good!

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

No branches or pull requests

2 participants