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

Migrate c.a.acs.c.search and c.a.acs.c.wrap.cqsearch to classic only bundle and deprecate #2760

Closed
3 tasks done
adamcin opened this issue Dec 14, 2021 · 6 comments · Fixed by #2771
Closed
3 tasks done

Comments

@adamcin
Copy link
Contributor

adamcin commented Dec 14, 2021

Required Information

com.day.cq.search.Query is now marked as @ProviderType interfaces in recent versions of cloudready sdks. This change makes the classes in com.adobe.acs.commons.search and com.adobe.acs.commons.wrap.cqsearch incompatible with AEM as a Cloud Service CQRules:CQBP-84 pipeline check.

  • AEM Version, including Service Packs, Cumulative Fix Packs, etc: Cloud Service
  • ACS AEM Commons Version: since introduction of CloseableQuery
  • Reproducible on Latest? yes

Expected Behavior

ACS AEM Commons should build without blocking code quality issues in AEM as a Cloud Service pipelines.

Actual Behavior

Recent builds fail CQRules:CQBP-84 with the following:

adobe/consulting:acs-aem-commons-ui.apps:5.0.14,0,	The product interface com.day.cq.search.Query annotated with 						@ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.search.CloseableQuery 												contained in 
adobe/consulting:acs-aem-commons-ui.apps:5.0.14,0,	The product interface com.day.cq.search.Query annotated with 						@ProviderType should not be implemented by custom code. Detected in com.adobe.acs.commons.wrap.cqsearch.QueryIWrap 		

Steps to Reproduce

Build the project in a code quality pipeline. see lines including the above.

#2604

@adamcin
Copy link
Contributor Author

adamcin commented Dec 14, 2021

My approach will be to:

  1. migrate the two packages to a new bundle, and deprecate the exported types, document new usage. force baseline goal to accept this migration without a major version bump of the existing bundle.
  2. do not embed the new bundle in ui.apps, so that existing classic/cloudservice projects can accept the change seamlesslly if they are not using CloseableQuery directly.
  3. create utility class in existing util package to less elegantly wrap a Query with an AutoCloseableQueryWrapper that implements AutoCloseable but which only provides the underlying Query with a get method. This should provide a workable solution for immediate refactoring in classic projects that do use CloseableQuery but would rather avoid adding a dependency on the new classic-only bundle.

@davidjgonzalez
Copy link
Contributor

@adamcin there has been some loose brainstorming of how we want to handle the CS / non-CS future of ACS Commons.

https://github.com/Adobe-Consulting-Services/acs-aem-commons/wiki

Loosely, creating 1 artifact for AEM CS deployments and another for Classic (AEM 6) - i expect well have to shade/copy any shared code or something - but it would be good to make sure what you're doing generally aligns w/ this (or doesn't create problems)... and/or if you have other ideas how to handle this more elegantly.

@davidjgonzalez
Copy link
Contributor

@adamcin This is a bit more extreme but ...

We could always remove these completely and bump to version 6.0.0 .... I can't imagine that many people are using them - and if they are, they can always copy the source into their own project.

Obviously, no one likes breaking back-compat, but i struggle to imagine that many ACS Commons-consumers are actually using either of these APIs....? Seems like a lot of work, additional complexity to the project itself, for an unlikely payoff.

If it becomes a big issue w/ 6.x we could always create an ancillary package when the issue arises?

@adamcin
Copy link
Contributor Author

adamcin commented Dec 15, 2021

@davidjgonzalez I was actually hoping you would be okay with that. I've deleted the packages in a local branch and the baseline goal doesn't consider it a breaking change. I think that we could safely remove them with my working PR and avoid bumping the project version.

@davidjgonzalez
Copy link
Contributor

that works for me :) ... I can add some extra notes to the release notes saying if you need these, migrate the code into your project and use it from there.

adamcin added a commit to adamcin/acs-aem-commons that referenced this issue Jan 14, 2022
adamcin added a commit to adamcin/acs-aem-commons that referenced this issue Jan 14, 2022
adamcin added a commit to adamcin/acs-aem-commons that referenced this issue Jan 14, 2022
@adamcin
Copy link
Contributor Author

adamcin commented Jan 14, 2022

@davidjgonzalez ready for review: #2771

davidjgonzalez pushed a commit that referenced this issue Jan 21, 2022
* re #2760 remove CloseableQuery from API
* code climate fixes
* add tests for QueryHelperImpl
* add test for QueryBuilderViewQuery
* resolves #2772 various blocker critical code scan issues
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 a pull request may close this issue.

2 participants