Skip to content

Fix :spring-authorization-server-docs:asciidoctor cacheability #1231

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

Closed

Conversation

jprinet
Copy link
Contributor

@jprinet jprinet commented May 24, 2023

Issues

  • The task is relying on inputs with absolute paths and therefore not getting cache hits when the cache is relocated.

  • The whole docs/src/docs/asciidoc folder is silently passed as an input to the task and this modifies the inputs depending on test task being fetched from build cache or not:

Screenshot 2023-05-23 at 4 38 38 PM

Fix

This PR:

  • converts entries from the attributes Map with absolute paths to relative paths (docs-java requires an absolute path and has to be moved into an asciidoc internal file attributes.adoc)
  • add an exclusion on AsciiDocTask resources

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 24, 2023
@sjohnr sjohnr self-assigned this May 24, 2023
@sjohnr
Copy link
Contributor

sjohnr commented May 24, 2023

Thanks @jprinet, appreciate the effort here!

  • The task is relying on inputs with absolute paths and therefore not getting cache hits when the cache is relocated.
  • The whole docs/src/docs/asciidoc folder is silently passed as an input to the task and this modifies the inputs depending on test task being fetched from build cache or not:

I'm not able to find information about this cache miss in a previous successful build (possibly due to unfamiliarity with Gradle Enterprise). Are you able to link to where you see the cache miss called out in the build?

This PR:

  • converts entries from the attributes Map with absolute paths to relative paths (docs-java requires an absolute path and has to be moved into an asciidoc internal file attributes.adoc)

The challenge I had with getting the docs set up originally was being able to edit docs locally in my IDE (with adoc file preview) while also having asciidoctor happy with all of the paths for resolution during a build. Asciidoctor picks a magical relative root for all paths to be resolved that the IDE has no idea about, which is quite frustrating.

The issue I'm finding with this change is that it breaks the relative pathing set up for preview. I'm also not entirely sure what it fixes. Can you explain why this change was necessary? I may not be fully understanding the need.

  • add an exclusion on AsciiDocTask resources

It seems like this is the real issue with cache misses, correct? Could we just relocate the examples folder to be outside the src/docs/asciidoc directory? Or is there something else that makes this necessary?

@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels May 24, 2023
@jprinet
Copy link
Contributor Author

jprinet commented May 25, 2023

I'm not able to find information about this cache miss in a previous successful build (possibly due to unfamiliarity with Gradle Enterprise). Are you able to link to where you see the cache miss called out in the build?

A cache miss due to inputs relying on absolute paths happens when relocating your cache, you can demonstrate it by running experiment 3 of the automated validation scripts.
The experience is about running 2 builds with the project cloned from 2 different locations on an empty cache. You will then get the following cache miss on the second build:

Screenshot 2023-05-25 at 9 51 37 AM

Please note that the PR also solves these deprecation warnings, as Gradle happens to identify some implicit dependencies between tasks likely due to this.

The challenge I had with getting the docs set up originally was being able to edit docs locally in my IDE (with adoc file preview) while also having asciidoctor happy with all of the paths for resolution during a build. Asciidoctor picks a magical relative root for all paths to be resolved that the IDE has no idea about, which is quite frustrating.

Not sure how to make the change compliant with your use case. Would keeping the examples-dir variable definition in the guides help there?

It seems like this is the real issue with cache misses, correct? Could we just relocate the examples folder to be outside the src/docs/asciidoc directory? Or is there something else that makes this necessary?

That is a part of the problem but wouldn't be sufficient afaik

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 25, 2023
sjohnr pushed a commit that referenced this pull request May 25, 2023
@jprinet
Copy link
Contributor Author

jprinet commented May 30, 2023

Which IDE / plugin are you using @sjohnr?
I have tried to reproduce locally with IntelliJ and the default AsciiDoc plugin, but both default and PR branch are leading to an exception.
Screenshot 2023-05-30 at 5 35 17 PM

@jprinet
Copy link
Contributor Author

jprinet commented May 30, 2023

I eventually found a way to make the plugin work in my environment, I also pushed a new commit which overrides the attributes in the subfolder.
Could you give it a try @sjohnr ?

@sjohnr sjohnr removed the status: feedback-provided Feedback has been provided label May 30, 2023
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 30, 2023
@sjohnr
Copy link
Contributor

sjohnr commented May 30, 2023

Yes, I'm using AsciiDoc plugin. I tried your new branch and that indeed seems to take care of it! I'm not sure why I was having trouble, as it seems straightforward now. Thanks so much!

On a related note, I'm only partially able to verify the cache miss issue with Gradle Enterprise builds. I tried the automated validation scripts but I seem to lack permissions on Gradle Enterprise to get any value out of it. I could potentially ask for more permissions, perhaps I'll do that at a later time. In the meantime, I think I'll revert the changes for the experiment for now.

Either way, I think I have enough info to incorporate this contribution, which I'll work on later this week. Thanks again!

sjohnr pushed a commit that referenced this pull request May 30, 2023
@jprinet
Copy link
Contributor Author

jprinet commented May 31, 2023

That's a good news, although I'm not super happy that an extra guides/attributes.adoc file is required only to make the IDE plugin happy.

With regards to the automated experiments, my understanding is that you are missing READ API permissions from Gradle Enterprise. This prevents the scripts from automatically fetching the experiment results.
You could ask for more permissions or investigate the experiment results by yourself by just checking that no cacheable task actually run in the second build. To do so, just filter the second build scan of the experiment like there.

@jgrandja jgrandja added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 31, 2023
@sjohnr
Copy link
Contributor

sjohnr commented May 31, 2023

That's a good news, although I'm not super happy that an extra guides/attributes.adoc file is required only to make the IDE plugin happy.

I set up the guides to be a subdirectory of the documentation site, which doesn't seem to be intended by asciidoctor, so it's likely my fault. I think this is only temporary as we will eventually switch to Antora which has a more well-defined project structure and a default location for examples. I believe it has its own IDE plugin as well.

sjohnr pushed a commit that referenced this pull request Jun 14, 2023
@sjohnr
Copy link
Contributor

sjohnr commented Jun 14, 2023

Thanks @jprinet! This is now merged to main as 2f7c4f6.

@sjohnr sjohnr closed this Jun 14, 2023
@sjohnr sjohnr added this to the 1.2.0-M1 milestone Jun 14, 2023
@jgrandja jgrandja modified the milestones: 1.2.0-M1, 1.2.0-M2 Aug 1, 2023
tjholmes66 added a commit to tjholmes66/spring-authorization-server that referenced this pull request Oct 2, 2023
* Update to io.spring.javaformat:spring-javaformat-checkstyle:0.0.35

Closes spring-projectsgh-1089

* Update to jackson-bom:2.14.2

Closes spring-projectsgh-1090

* Update to junit-jupiter:5.9.2

Closes spring-projectsgh-1091

* Release 1.0.1

* Next Development Version

* Update to Spring Security 6.1.0-M1

Closes spring-projectsgh-1093

* Update to nimbus-jose-jwt:9.30.2

Closes spring-projectsgh-1094

* Update to assertj-core:3.24.2

Closes spring-projectsgh-1095

* Update to mockito-core:4.11.0

Closes spring-projectsgh-1096

* Release 1.1.0-M1

* Next Development Version

* Add user property to deploy_docs workflow

* Fix broken support link

Closes spring-projectsgh-1092

* Fix client secret encoding when client dynamically registered

Closes spring-projectsgh-1056

* Polish spring-projectsgh-1056

* Allow PasswordEncoder to be configured in OidcClientRegistrationAuthenticationProvider

Issue spring-projectsgh-1056

* Upgrade client secret when available

Closes spring-projectsgh-1099

* Polish spring-projectsgh-1105

* Add support for OAuth 2.0 Device Authorization Grant

Closes spring-projectsgh-44

* Switch to spring-security SNAPSHOT dependencies

Issue spring-projectsgh-44

* Use spring-security 6.1 in snapshot tests

Issue spring-projectsgh-1106

* Update to actions/checkout@v3

Closes spring-projectsgh-1117

* Use spring-io/spring-gradle-build-action

Closes spring-projectsgh-1120

* Use spring-io/spring-gradle-build-action

Closes spring-projectsgh-1120

* Revert accidental change in versions

Issue spring-projectsgh-1120

* Polish spring-projectsgh-1106

* Update to Spring Framework 6.0.7

Closes spring-projectsgh-1130

* Update to Spring Security 1.1.0-M2

Closes spring-projectsgh-1131

* Update to nimbus-jose-jwt:9.31

Closes spring-projectsgh-1132

* Update to Spring Framework 6.0.7 in buildSrc

Issue spring-projectsgh-1130

* Release 1.1.0-M2

* Next Development Version

* Polish spring-projectsgh-1106 Device Authorization Grant

* Avoid persisting client principal in device authorization request

Issue spring-projectsgh-1106

* Polish spring-projectsgh-1068

Issue spring-projectsgh-1077

* Add OidcLogoutAuthenticationToken.isPrincipalAuthenticated()

Issue spring-projectsgh-1077

* Ensure ID Token is active before processing logout request

Issue spring-projectsgh-1077

* Allow localhost in redirect_uri

Closes spring-projectsgh-651

* Fix refresh token error code INVALID_CLIENT to INVALID_GRANT

Closes spring-projectsgh-1139

* Do not require authorizationRequest for device grant

Issue spring-projectsgh-1127

* Add tests for OAuth 2.0 Device Authorization Grant

This commit adds tests for the following components:
* AuthenticationConverters
* AuthenticationProviders
* Endpoint Filters

Issue spring-projectsgh-44
Closes spring-projectsgh-1127

* JDBC device_code authorization

Issue spring-projectsgh-1156

* Polish spring-projectsgh-1143

* Add tests and update examples in docs

Closes spring-projectsgh-1156

* Polish ref-doc

Issue spring-projectsgh-1156

* Add customization to support public clients for device grant

Issue spring-projectsgh-1157

* Polish samples

Closes spring-projectsgh-1157

* Add documentation for OAuth 2.0 Device Authorization Grant

Closes spring-projectsgh-1158

* Polish spring-projectsgh-1127

* Polish spring-projectsgh-1158

* Add documentation for OpenID Connect 1.0 Logout Endpoint

Closes spring-projectsgh-1069

* Update Stack Overflow tag to spring-authorization-server

* Update to Spring Framework 5.3.27

Closes spring-projectsgh-1162

* Update to Spring Security 5.8.3

Closes spring-projectsgh-1163

* Update to io.spring.javaformat:spring-javaformat-checkstyle:0.0.38

Closes spring-projectsgh-1164

* Update to Spring Framework 6.0.8

Closes spring-projectsgh-1165

* Update to Spring Security 6.0.3

Closes spring-projectsgh-1166

* Update to io.spring.javaformat:spring-javaformat-checkstyle:0.0.38

Closes spring-projectsgh-1167

* Update to Spring Framework 6.0.8

Closes spring-projectsgh-1168

* Update to Spring Security 6.1.0-RC1

Closes spring-projectsgh-1169

* Update to io.spring.javaformat:spring-javaformat-checkstyle:0.0.38

Closes spring-projectsgh-1170

* Update to json-path:2.8.0

Closes spring-projectsgh-1171

* Release 0.4.2

* Next Development Version

* Release 1.0.2

* Next Development Version

* Release 1.1.0-RC1

* Next Development Version

* Update to org.jfrog.buildinfo:build-info-extractor-gradle:4.29.0

Closes spring-projectsgh-1175

* Apply ArtifactoryPlugin to SpringRootProjectPlugin

Closes spring-projectsgh-1177

* Fix artifact build properties for Artifactory

- Apply SpringArtifactoryPlugin in SpringRootProjectPlugin (which applies ArtifactoryPlugin)
- In SpringArtifactoryPlugin don't set publication if MavenPublishPlugin is not applied

Closes spring-projectsgh-1179

* Add test for dynamic client registration with custom metadata

Issue spring-projectsgh-1172

* Add logout success page to default client sample

Sample client (located in 'samples/messages-client' directory) is configured with a custom logout success page where
the end-user is redirected to upon successful logout action.

Fixes spring-projectsgh-1142

* Add sample featured-authorizationserver

Issue spring-projectsgh-1189

* Merge custom-consent-authorizationserver into featured-authorizationserver

Issue spring-projectsgh-1189

* Merge federated-identity-authorizationserver into featured-authorizationserver

Issue spring-projectsgh-1189

* Update io.spring.ge.conventions plugin to 0.0.13

Closes spring-projectsgh-1190

* Update spring-asciidoctor-backends to 0.0.5

Closes spring-projectsgh-1192

* Merge device-grant-authorizationserver into featured-authorizationserver

Issue spring-projectsgh-1189

* Merge device-client into messages-client

Issue spring-projectsgh-1189

* Use custom consent page for device code flow

Issue spring-projectsgh-1189

* Use current authentication for device authorization

Issue spring-projectsgh-1189

* Reuse error handling

Issue spring-projectsgh-1189

* Handle web client response error

Issue spring-projectsgh-1189

* Update @SInCE

* Rename featured-authorizationserver to demo-authorizationserver

Issue spring-projectsgh-1189

* Rename messages-client to demo-client

Issue spring-projectsgh-1189

* Update sample README

Issue spring-projectsgh-1189

* Add integration tests for device grant

Issue spring-projectsgh-1116

* Update web ui design for demo-client

Issue spring-projectsgh-1196

* Polish web ui design for demo-client

Issue spring-projectsgh-1196

* Update web ui design for demo-authorizationserver

Issue spring-projectsgh-1196

* Polish web ui design for demo-client

Issue spring-projectsgh-1196

* Polish demo sample

Issue spring-projectsgh-1189

* Update to Spring Boot 3.1.0-RC1

Closes spring-projectsgh-1198

* Refresh Getting Started example

Closes spring-projectsgh-1186

* Use Spring Boot starter in samples

Closes spring-projectsgh-1187

* Invalidate tokens previously issued when code is reused

Closes spring-projectsgh-1152

* Polish spring-projectsgh-1152

* Add How-to: Authenticate using Social Login

Closes spring-projectsgh-538

* Add How-to: Authenticate using a Single Page Application with PKCE

Closes spring-projectsgh-539

* Hash the sid claim in the ID Token

Closes spring-projectsgh-1207

* Simplified federated login in demo sample

Closes spring-projectsgh-1208

* Polish spring-projectsgh-1186

* Polish spring-projectsgh-538

* Remove FederatedIdentityConfigurer from demo sample

Issue spring-projectsgh-1208

* Update exception handling config in ref-doc

Closes spring-projectsgh-1205

* Update exception handling config in samples

Closes spring-projectsgh-1204

* Polish ref-doc

Issue spring-projectsgh-1205

* Polish tests

* Add How-to: Implement an Extension Authorization Grant Type

Closes spring-projectsgh-686

* Update to Spring Framework 6.0.9

Closes spring-projectsgh-1213

* Update to Spring Security 6.1.0

Closes spring-projectsgh-1214

* Update to jackson-bom 2.15.0

Closes spring-projectsgh-1215

* Update to junit-jupiter 5.9.3

Closes spring-projectsgh-1216

* Release 1.1.0

* Next Development Version

* Revert serialVersionUID to 0.4.0

Closes spring-projectsgh-1218

* Revert serialVersionUID to 1.0.0

Closes spring-projectsgh-1219

* Revert serialVersionUID to 1.1.0

Closes spring-projectsgh-1220

* Exclude project dependency from spring-boot-dependencies

Closes spring-projectsgh-1228

* Update to Spring Boot 3.1.0

* Update com.gradle.enterprise plugin to 3.13.3

Closes spring-projectsgh-1234
Issue spring-projectsgh-1231

* Prepare for automated validation scripts

See https://github.com/gradle/gradle-enterprise-build-validation-scripts/blob/main/Gradle.md

Issue spring-projectsgh-1231

* ID Token contains sid claim after refresh_token grant

Closes spring-projectsgh-1224

* Use substring instead of replaceFirst in OAuth2AuthorizationConsent

Closes spring-projectsgh-1222

* Validate authorized principal instead of sub during logout

Closes spring-projectsgh-1235

* Revert "Prepare for automated validation scripts"

This reverts commit ece9f10.

Issue spring-projectsgh-1231

* Add debug log entries

Closes spring-projectsgh-1245
Closes spring-projectsgh-1246
Closes spring-projectsgh-1247
Closes spring-projectsgh-1248

* Polish additional logging

Issue spring-projectsgh-1245, spring-projectsgh-1246, spring-projectsgh-1247, spring-projectsgh-1248

* Enable caching of :asciidoctor gradle task

* Use direct code import

Issue spring-projectsgh-1231

* Next Minor Version

* Fix NPE on access token in OAuth2AuthorizationCodeAuthenticationProvider

Closes spring-projectsgh-1233

* Polish spring-projectsgh-1233

* Fix to save all values for multi-valued request parameters

Fixes spring-projectsgh-1250

* Polish spring-projectsgh-1252

* Fix to save all values for multi-valued device grant parameters

Fixes spring-projectsgh-1269

* Polish spring-projectsgh-1252

* Update to Spring Framework 5.3.28

Closes spring-projectsgh-1272

* Update to Spring Security 5.8.4

Closes spring-projectsgh-1273

* Update to jackson-bom 2.14.3

Closes spring-projectsgh-1274

* Update to Spring Framework 6.0.10

Closes spring-projectsgh-1276

* Update to Spring Security 6.0.4

Closes spring-projectsgh-1277

* Update to Spring Framework 6.0.10

Closes spring-projectsgh-1278

* Update to Spring Security 6.1.1

Closes spring-projectsgh-1279

* Update to junit-jupiter 5.9.3

Closes spring-projectsgh-1280

* Update to junit-jupiter 5.9.3

Closes spring-projectsgh-1281

* Update to jackson-bom 2.15.2

Closes spring-projectsgh-1282

* Update feature planning section to using GitHub Projects

* Release 1.1.1

* Next Development Version

* Fix generating ID token with null sid when refresh_token grant

Closes spring-projectsgh-1283

* Polish spring-projectsgh-1289

* Fix NPE in DefaultErrorController

Closes spring-projectsgh-1286

* Migrate docs to Antora

Issue spring-projectsgh-1295

* Polish Antora migration

Issue spring-projectsgh-1292
Closes spring-projectsgh-1295

* Remove unused antora-playbook.yml

* Replaces 'install' with 'publishToMavenLocal' command in README

* Adds how-to guide on adding authorities to access tokens

Closes spring-projectsgh-542

* Polish spring-projectsgh-1264

* Update order of guides in nav.adoc

* Fix userCode validation

Issue spring-projectsgh-44

* Polish spring-projectsgh-1309

* Add Revved up by Gradle Enterprise badge

* Move badges to header

This is similar to Spring Boot:
  https://github.com/spring-projects/spring-boot/blob/main/README.adoc

* Fix workflow status link

* Fix samples test suite execution and failing tests

Closes spring-projectsgh-1324

* Polish spring-projectsgh-1325

* Move deploy-docs.yml to correct folder

Issue spring-projectsgh-1295

* Remove manual list of guides

Issue spring-projectsgh-1295

* Remove reference to gh-pages

Issue spring-projectsgh-1295

* Update to Spring Framework 6.0.11

Closes spring-projectsgh-1338

* Update to Spring Security 6.1.2

Closes spring-projectsgh-1339

* Update to org.hsqldb:hsqldb 2.7.2

Closes spring-projectsgh-1340

* Release 1.1.2

* Next Development Version

* Adds ability to inject custom metadata at client registration

Closes spring-projectsgh-1172

* Polish spring-projectsgh-1326

* Adds dynamic client registration how-to guide

Closes spring-projectsgh-647

* Polish spring-projectsgh-1320

* Add code challenge methods for oidc provider configuration response

Closes spring-projectsgh-1302

* Update to Spring Framework 6.1.0-M5

Closes spring-projectsgh-1364

* Update to Spring Security 6.2.0-M3

Closes spring-projectsgh-1365

* Update to nimbus-jose-jwt 9.35

Closes spring-projectsgh-1366

* Update to junit-jupiter 5.10.0

Closes spring-projectsgh-1367

* Update to okhttp 4.11.0

Closes spring-projectsgh-1368

* Release 1.2.0-M1

* Next Development Version

---------

Co-authored-by: Joe Grandja <[email protected]>
Co-authored-by: Siva Kumar Edupuganti <[email protected]>
Co-authored-by: Yuta Saito <[email protected]>
Co-authored-by: Shannon Pamperl <[email protected]>
Co-authored-by: Steve Riesenberg <[email protected]>
Co-authored-by: HuiYeong <[email protected]>
Co-authored-by: Xu Xiaowei <[email protected]>
Co-authored-by: Janne Valkealahti <[email protected]>
Co-authored-by: Dmitriy Dubson <[email protected]>
Co-authored-by: neochae <[email protected]>
Co-authored-by: heartape <[email protected]>
Co-authored-by: Dejan Varmedja <[email protected]>
Co-authored-by: Jerome Prinet <[email protected]>
Co-authored-by: Pavel Efros <[email protected]>
Co-authored-by: Martin Lindström <[email protected]>
Co-authored-by: cbilodeau <[email protected]>
Co-authored-by: Rob Winch <[email protected]>
Co-authored-by: Dmitriy Dubson <[email protected]>
Co-authored-by: Martin Bogusz <[email protected]>
Co-authored-by: Eric Haag <[email protected]>
Co-authored-by: Tuxzx <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants