codeconsole commented on code in PR #16142: URL: https://github.com/apache/grails-core/pull/16142#discussion_r3779217340
########## grails-gsp/core/src/test/groovy/org/grails/gsp/compiler/GroovyPageCompilerReproducibilitySpec.groovy: ########## @@ -0,0 +1,109 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.gsp.compiler + +import spock.lang.Specification +import spock.lang.TempDir + +import org.grails.gsp.GroovyPageMetaInfo + +/** + * Precompiled GSPs must not vary with the modification time of their source. + * + * Git records no modification times, so every fresh clone or CI checkout gives each .gsp a new one. Baking + * that into the generated class made otherwise identical jars differ on every checkout, and because + * {@code LAST_MODIFIED} was a compile-time constant the difference survived Gradle's compile-classpath + * normalization, so every downstream task missed the build cache. + */ +class GroovyPageCompilerReproducibilitySpec extends Specification { + + private static final String PAGE_CONTENT = '<html><body><g:if test="${flag}">Hello</g:if></body></html>' + + @TempDir + File tempDir + + private File viewsDir + private File page + + void setup() { + this.viewsDir = new File(this.tempDir, 'views') + this.page = new File(this.viewsDir, 'index.gsp') + this.page.parentFile.mkdirs() + this.page.text = PAGE_CONTENT + } + + void 'a source compiled at two different modification times produces identical classes'() { + when: 'the same page is compiled twice, as two checkouts of one commit would' + this.page.setLastModified(1_000_000_000_000L) + byte[] first = compileToBytes('first') + + and: + this.page.setLastModified(1_700_000_000_000L) + byte[] second = compileToBytes('second') + + then: 'the jar built from them is byte-identical, so downstream tasks keep their cache hits' + first == second + } + + void 'a compiled page records a checksum of its source instead of a modification time'() { + when: + GroovyPageMetaInfo metaInfo = compileToMetaInfo('recorded') + + then: 'the checksum identifies the content' + metaInfo.sourceChecksum ==~ /[0-9a-f]{64}/ + + and: 'no modification time is baked in for a fresh checkout to invalidate' + metaInfo.lastModified == 0L + } + + void 'an edited source produces a different checksum'() { + given: + GroovyPageMetaInfo before = compileToMetaInfo('before') + + when: + this.page.text = '<html><body>something else entirely</body></html>' + GroovyPageMetaInfo after = compileToMetaInfo('after') + + then: 'the runtime can still tell that the page changed' + before.sourceChecksum != after.sourceChecksum + } + + private Map compile(File targetDir) { + targetDir.mkdirs() + GroovyPageCompiler compiler = new GroovyPageCompiler() + compiler.viewsDir = this.viewsDir + compiler.srcFiles = [this.page] + compiler.targetDir = targetDir + compiler.generatedGroovyPagesDirectory = new File(this.tempDir, 'generated').tap { mkdirs() } + compiler.compile() + } + + private byte[] compileToBytes(String name) { + File targetDir = new File(this.tempDir, name) + Map results = compile(targetDir) + new File(targetDir, "${results.values().first()}.class").bytes + } + + private GroovyPageMetaInfo compileToMetaInfo(String name) { + File targetDir = new File(this.tempDir, name) + Map results = compile(targetDir) + ClassLoader loader = new URLClassLoader([targetDir.toURI().toURL()] as URL[], getClass().classLoader) + new GroovyPageMetaInfo(loader.loadClass(results.values().first() as String)) Review Comment: Applied — now `withCloseable`. Both uses complete inside the closure, so there was no reason to hold the loader open. ########## grails-gsp/core/src/test/groovy/org/grails/gsp/GroovyPageMetaInfoReloadSpec.groovy: ########## @@ -0,0 +1,180 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.gsp + +import java.security.PrivilegedAction + +import spock.lang.Specification +import spock.lang.TempDir + +import org.springframework.core.io.FileSystemResource +import org.springframework.core.io.Resource + +import org.grails.gsp.compiler.GroovyPageParser + +/** + * Reload-staleness behaviour of {@link GroovyPageMetaInfo}. + * + * A page compiled by {@code GroovyPageCompiler} records a checksum of its source rather than the source's + * modification time, which git does not preserve across a checkout. Staleness is therefore decided by + * comparing content, falling back to the timestamp for pages compiled before the checksum existed. + * + * Each feature uses a fresh {@code GroovyPageMetaInfo}, because the result of a check is cached for + * {@code grails.gsp.reload.interval} milliseconds. + */ +class GroovyPageMetaInfoReloadSpec extends Specification { + + private static final String PAGE_CONTENT = '<html><body>hi</body></html>' + + @TempDir + File tempDir + + private Resource sourcePage(String content = PAGE_CONTENT) { + File page = new File(this.tempDir, 'index.gsp') + page.text = content + new FileSystemResource(page) + } + + private static String checksumOf(Resource resource) { + resource.inputStream.withStream { InputStream input -> GroovyPageParser.checksumOf(input) } + } + + private static PrivilegedAction<Resource> callableFor(Resource resource) { + { -> resource } as PrivilegedAction + } + + void 'a page whose recorded checksum matches its source is not reported as stale'() { + given: 'a precompiled page recording the checksum of the source on disk' + Resource resource = sourcePage() + GroovyPageMetaInfo metaInfo = new GroovyPageMetaInfo() + metaInfo.sourceChecksum = checksumOf(resource) + + expect: + !metaInfo.shouldReload(callableFor(resource)) + } + + void 'a page whose source no longer matches its recorded checksum is reported as stale'() { + given: 'a precompiled page whose source has since been edited' + Resource resource = sourcePage() + GroovyPageMetaInfo metaInfo = new GroovyPageMetaInfo() + metaInfo.sourceChecksum = checksumOf(resource) + resource.getFile().text = '<html><body>edited</body></html>' + + expect: + metaInfo.shouldReload(callableFor(resource)) + } + + void 'a source that was touched but not edited is not reported as stale'() { + given: 'a page whose source carries a modification time nothing like the one it was compiled at' + Resource resource = sourcePage() + GroovyPageMetaInfo metaInfo = new GroovyPageMetaInfo() + metaInfo.sourceChecksum = checksumOf(resource) + resource.getFile().setLastModified(resource.getFile().lastModified() + 86_400_000L) Review Comment: Applied at all three sites (88, 113, 126). Agreed this is the worse variant of the same problem: the features would have kept passing via the checksum path while quietly no longer exercising the scenario their names claim. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
