codeconsole commented on code in PR #16134:
URL: https://github.com/apache/grails-core/pull/16134#discussion_r3793083792
##########
grails-gsp/grails-web-taglib/src/main/groovy/grails/artefact/gsp/TagLibraryInvoker.groovy:
##########
@@ -93,11 +93,14 @@ trait TagLibraryInvoker extends WebAttributes {
}
if (tagLibrary) {
- if (!developmentMode) {
- MetaClass thisMc =
GrailsMetaClassUtils.getMetaClass(this)
-
TagLibraryMetaUtils.registerMethodMissingForTags(thisMc, lookup, usedNamespace,
methodName)
- }
- return tagLibrary.invokeMethod(methodName, args)
+ // Resolving the tag used to install it onto this object's
metaclass so that later
+ // calls bypassed methodMissing. That made every caller
mutate its own
+ // ExpandoMetaClass the first time it used a tag, and made
every later call pay the
+ // read lock guarding an initialised metaclass. The tag is
dispatched through the
+ // lookup each time instead, which is a map read.
+ return TagLibraryMetaUtils.methodMissingForTagLib(
Review Comment:
You're right that it's a behaviour change to a public trait.
`TagLibraryInvokerDispatchSpec` now covers a method-declared tag called
unqualified through `methodMissing` from a class carrying the trait, asserting
the captured output is returned rather than the method's own return value, and
it's in `upgrading80x.adoc` with a before/after.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPage.java:
##########
@@ -296,6 +310,23 @@ public Object getProperty(String property) {
return resolveProperty(property);
}
+ /**
+ * Resolves a tag called without a namespace, as {@code ${message(code:
'x')}} is.
+ *
+ * <p>A real method rather than one installed onto this page's metaclass.
Installing it, along with
+ * a method for every tag and a property for every namespace, meant
writing to an
+ * ExpandoMetaClass for every page compiled and made every later tag call
a read of an initialised
+ * metaclass, which is guarded by a lock.
+ *
+ * @param name the tag name
+ * @param args the arguments the tag was called with
+ * @return whatever the tag produces
+ */
+ public Object methodMissing(String name, Object args) {
+ return TagLibraryMetaUtils.methodMissingForTagLib(getMetaClass(),
getClass(), gspTagLibraryLookup,
Review Comment:
I could not reproduce this. With a verified-null lookup and the guard
removed, `MissingMethodException` is thrown, not an NPE — the dynamic call on
the null receiver yields no tag library rather than throwing, so it falls
through to the `MissingMethodException` at the end of `methodMissingForTagLib`.
Worth flagging what misled me first time: my initial spec passed against the
unguarded code, and the exception carried `arguments == [null]`, which comes
from `makeObjectArray` — so the guard was never reached. Property access on a
`GroovyPage` routes through its overridden `getProperty`, so
`page.tagLibraryLookup` wasn't reading the field at all.
I kept a guard anyway, since the field is documented as null before
initialisation and relying on Groovy's null-receiver behaviour isn't a
contract, but reworded so it doesn't claim to fix a crash.
`GroovyPageMethodMissingSpec` pins the behaviour. Happy to drop it.
##########
grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/index/TagLibraryIndex.java:
##########
@@ -0,0 +1,485 @@
+/*
+ * 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.taglib.index;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.Reader;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.WeakHashMap;
+
+/**
+ * The set of tag libraries and tag names known at compile time.
+ *
+ * <p>Each tag library contributes one descriptor under {@value
#INDEX_LOCATION}, written by the
+ * {@code TagLib} AST transformation as the tag library is compiled.
Descriptors are per class rather
+ * than per module so that libraries packaged in separate jars merge on the
classpath without any
+ * build step having to combine them, in the same way {@code
META-INF/services} entries do.
+ *
+ * <p>Reading the index answers "which tags exist in namespace x" without
loading or reflecting over a
+ * single tag library class, which is what allows GSP expressions to be
resolved when a page is
+ * compiled rather than dispatched dynamically when it renders.
+ *
+ * @since 8.0.0
+ */
+public final class TagLibraryIndex {
+
+ /**
+ * Classpath directory holding one descriptor per compiled tag library.
+ */
+ public static final String INDEX_LOCATION = "META-INF/grails/taglibs/";
+
+ /**
+ * Descriptor format this build writes and understands. A descriptor
carrying anything else was
+ * produced by a different version of Grails and is ignored, so its tags
resolve dynamically rather
+ * than being read under the wrong set of rules.
+ */
+ public static final int FORMAT_VERSION = 2;
+
+ /**
+ * Settings the build states for the compilation the index is read in,
written alongside the
+ * descriptors by the build and deliberately not packaged into the
artifact: they describe how this
+ * project is compiled, not what its tag libraries declare.
+ */
+ public static final String SETTINGS_LOCATION = INDEX_LOCATION +
"compile-settings.properties";
+
+ /**
+ * What the index could not describe, written by whatever produced it.
+ *
+ * <p>An index generated before its project is compiled cannot always read
every tag library: one
+ * referring to a type that does not exist yet, in a language it cannot
parse, or generated by the
+ * build itself, is left out. A namespace missing some of its tags must
not have a call to one of
+ * them reported as a misspelling, so what was missed is recorded rather
than left to be inferred
+ * from the absence.
+ */
+ public static final String INCOMPLETE_LOCATION = INDEX_LOCATION +
"incomplete.properties";
Review Comment:
Documented. The guide now has a "when strict checking can be used" section
saying plainly that a namespace only some of its jars described cannot be
checked, that this is the normal state of `g`, and that an application
depending on an undescribed third-party tag library in `g` is better off
leaving the default alone than naming `g` in `dynamicTagNamespaces`.
##########
grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/index/TagLibraryIndex.java:
##########
@@ -0,0 +1,485 @@
+/*
+ * 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.taglib.index;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.Reader;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.WeakHashMap;
+
+/**
+ * The set of tag libraries and tag names known at compile time.
+ *
+ * <p>Each tag library contributes one descriptor under {@value
#INDEX_LOCATION}, written by the
+ * {@code TagLib} AST transformation as the tag library is compiled.
Descriptors are per class rather
+ * than per module so that libraries packaged in separate jars merge on the
classpath without any
+ * build step having to combine them, in the same way {@code
META-INF/services} entries do.
+ *
+ * <p>Reading the index answers "which tags exist in namespace x" without
loading or reflecting over a
+ * single tag library class, which is what allows GSP expressions to be
resolved when a page is
+ * compiled rather than dispatched dynamically when it renders.
+ *
+ * @since 8.0.0
+ */
+public final class TagLibraryIndex {
+
+ /**
+ * Classpath directory holding one descriptor per compiled tag library.
+ */
+ public static final String INDEX_LOCATION = "META-INF/grails/taglibs/";
+
+ /**
+ * Descriptor format this build writes and understands. A descriptor
carrying anything else was
+ * produced by a different version of Grails and is ignored, so its tags
resolve dynamically rather
+ * than being read under the wrong set of rules.
+ */
+ public static final int FORMAT_VERSION = 2;
+
+ /**
+ * Settings the build states for the compilation the index is read in,
written alongside the
+ * descriptors by the build and deliberately not packaged into the
artifact: they describe how this
+ * project is compiled, not what its tag libraries declare.
+ */
+ public static final String SETTINGS_LOCATION = INDEX_LOCATION +
"compile-settings.properties";
+
+ /**
+ * What the index could not describe, written by whatever produced it.
+ *
+ * <p>An index generated before its project is compiled cannot always read
every tag library: one
+ * referring to a type that does not exist yet, in a language it cannot
parse, or generated by the
+ * build itself, is left out. A namespace missing some of its tags must
not have a call to one of
+ * them reported as a misspelling, so what was missed is recorded rather
than left to be inferred
+ * from the absence.
+ */
+ public static final String INCOMPLETE_LOCATION = INDEX_LOCATION +
"incomplete.properties";
+
+ static final String VERSION_KEY = "version";
+ static final String NAMESPACE_KEY = "namespace";
+ static final String CLASS_KEY = "class";
+ static final String TAGS_KEY = "tags";
+ static final String STRICT_KEY = "strictTags";
+ static final String INCOMPLETE_NAMESPACES_KEY = "namespaces";
+ static final String INCOMPLETE_ALL_KEY = "all";
+ static final String DYNAMIC_NAMESPACES_KEY = "dynamicTagNamespaces";
+
+ /**
+ * One index per class loader. A compilation gets a class loader of its
own, so this is read once
+ * per compilation rather than once per source file, and is not held after
that compilation ends.
+ * Caching in a plain static field instead would carry one project's tag
libraries into the next
+ * compilation in the same Gradle daemon.
+ */
+ private static final Map<ClassLoader, TagLibraryIndex> BY_CLASS_LOADER =
+ Collections.synchronizedMap(new WeakHashMap<>());
+
+ private final Map<String, Map<String, TagLibraryIndexEntry>> byNamespace;
+ private final Map<String, Set<String>> ambiguousByNamespace;
+ private final Map<String, Set<String>> tagNamesByClass;
+ private final boolean strict;
+ private final Set<String> dynamicNamespaces;
+ private final Set<String> incompleteNamespaces;
+ private final boolean everythingIncomplete;
+
+ private TagLibraryIndex(Map<String, Map<String, TagLibraryIndexEntry>>
byNamespace,
+ Map<String, Set<String>> ambiguousByNamespace, Map<String,
Set<String>> tagNamesByClass,
+ boolean strict, Set<String> dynamicNamespaces, Set<String>
incompleteNamespaces,
+ boolean everythingIncomplete) {
+ this.byNamespace = byNamespace;
+ this.ambiguousByNamespace = ambiguousByNamespace;
+ this.tagNamesByClass = tagNamesByClass;
+ this.strict = strict;
+ this.dynamicNamespaces = dynamicNamespaces;
+ this.incompleteNamespaces = incompleteNamespaces;
+ this.everythingIncomplete = everythingIncomplete;
+ }
+
+ /**
+ * Reads the index for a class loader, reusing the one already read for it.
+ *
+ * <p>Reading walks every jar on the classpath, so a compiler that
consults the index for each
+ * source file it compiles would walk it once per file. Use this from
compilation; use
+ * {@link #load(ClassLoader)} where a fresh read is wanted.
+ *
+ * @param classLoader the loader to scan; when {@code null} the thread
context loader is used
+ * @return the merged index, never {@code null}
+ */
+ public static TagLibraryIndex forClassLoader(ClassLoader classLoader) {
+ ClassLoader loader = classLoader != null ? classLoader :
Thread.currentThread().getContextClassLoader();
+ if (loader == null) {
+ return load(null);
+ }
+ return BY_CLASS_LOADER.computeIfAbsent(loader, TagLibraryIndex::load);
+ }
+
+ /**
+ * Reads every tag library descriptor visible to the given class loader.
+ *
+ * @param classLoader the loader to scan; when {@code null} the thread
context loader is used
+ * @return the merged index, never {@code null}
+ */
+ public static TagLibraryIndex load(ClassLoader classLoader) {
+ ClassLoader loader = classLoader != null ? classLoader :
Thread.currentThread().getContextClassLoader();
+ Map<String, Map<String, TagLibraryIndexEntry>> merged = new
TreeMap<>();
+ Map<String, Set<String>> ambiguous = new TreeMap<>();
+ Map<String, Set<String>> byClass = new TreeMap<>();
+ if (loader == null) {
+ return new TagLibraryIndex(merged, ambiguous, byClass, false,
Collections.emptySet(),
+ Collections.emptySet(), false);
+ }
+ // A directory resource enumerates its children on some classpath
layouts but not inside jars,
+ // so the descriptors are discovered through the manifest of names
each descriptor records
+ // rather than by listing the directory.
+ for (URL url : listDescriptors(loader)) {
+ Properties properties = read(url);
+ if (properties == null) {
+ continue;
+ }
+ if
(!String.valueOf(FORMAT_VERSION).equals(properties.getProperty(VERSION_KEY))) {
+ continue;
+ }
+ String namespace = properties.getProperty(NAMESPACE_KEY);
+ String className = properties.getProperty(CLASS_KEY);
+ String tags = properties.getProperty(TAGS_KEY, "");
+ if (namespace == null || namespace.isEmpty() || className == null
|| className.isEmpty()) {
+ continue;
+ }
+ // Recorded from the descriptor rather than from its tags, so that
a tag library declaring
+ // none of them is still known to have been described. Deciding
that from the tags alone
+ // would have such a tag library described twice.
+ byClass.computeIfAbsent(className, k -> new TreeSet<>());
+ Map<String, TagLibraryIndexEntry> tagsForNamespace =
+ merged.computeIfAbsent(namespace, k -> new TreeMap<>());
+ for (String encodedTag : tags.split(",")) {
+ String trimmed = encodedTag.trim();
+ if (trimmed.isEmpty()) {
+ continue;
+ }
+ // Recorded as "name:KIND"; an unrecognised kind is treated as
the dynamic one so that a
+ // descriptor from a later version cannot cause a call to be
bound wrongly.
+ int separator = trimmed.lastIndexOf(':');
+ String tagName = separator > 0 ? trimmed.substring(0,
separator) : trimmed;
+ TagLibraryIndexEntry.Kind kind =
TagLibraryIndexEntry.Kind.LEGACY_CLOSURE;
+ if (separator > 0) {
+ try {
+ kind =
TagLibraryIndexEntry.Kind.valueOf(trimmed.substring(separator + 1));
+ } catch (IllegalArgumentException unknownKind) {
+ kind = TagLibraryIndexEntry.Kind.LEGACY_CLOSURE;
+ }
+ }
+ trimmed = tagName;
+ // Recorded against the declaring class before ambiguity is
considered, so that asking
+ // what one tag library declares is answered from its own
descriptor and is unaffected
+ // by whether some other tag library happens to declare the
same name.
+ byClass.computeIfAbsent(className, k -> new
TreeSet<>()).add(trimmed);
+ TagLibraryIndexEntry existing = tagsForNamespace.get(trimmed);
+ if (existing != null &&
!existing.tagLibraryClassName().equals(className)) {
+ // At runtime the tag library registered last wins, and
registration order comes
+ // from artefact scanning rather than from classpath
order, so which of these two
+ // will win cannot be known here. Resolving it either way
risks compiling against
+ // one implementation and dispatching to the other, so the
tag is marked ambiguous
+ // and left to runtime resolution.
+ ambiguous.computeIfAbsent(namespace, k -> new
TreeSet<>()).add(trimmed);
+ continue;
+ }
+ tagsForNamespace.put(trimmed,
+ new TagLibraryIndexEntry(namespace, trimmed,
className, kind, true));
+ }
+ }
+ Properties settings = readSettings(loader);
+ boolean strict = Boolean.parseBoolean(settings.getProperty(STRICT_KEY,
"false"));
+ Set<String> dynamic = new TreeSet<>();
+ for (String namespace : settings.getProperty(DYNAMIC_NAMESPACES_KEY,
"").split(",")) {
+ String trimmed = namespace.trim();
+ if (!trimmed.isEmpty()) {
+ dynamic.add(trimmed);
+ }
+ }
+ Set<String> incomplete = new TreeSet<>();
+ boolean allIncomplete = false;
+ for (URL url : urls(loader, INCOMPLETE_LOCATION)) {
+ Properties recorded = read(url);
+ if (recorded == null) {
+ continue;
+ }
+ allIncomplete |=
Boolean.parseBoolean(recorded.getProperty(INCOMPLETE_ALL_KEY, "false"));
+ for (String namespace :
recorded.getProperty(INCOMPLETE_NAMESPACES_KEY, "").split(",")) {
+ String trimmed = namespace.trim();
+ if (!trimmed.isEmpty()) {
+ incomplete.add(trimmed);
+ }
+ }
+ }
+ return new TagLibraryIndex(merged, ambiguous, byClass, strict,
+ Collections.unmodifiableSet(dynamic),
Collections.unmodifiableSet(incomplete),
+ allIncomplete);
+ }
+
+ private static Set<URL> urls(ClassLoader loader, String location) {
+ Set<URL> found = new LinkedHashSet<>();
+ try {
+ Enumeration<URL> resources = loader.getResources(location);
+ while (resources.hasMoreElements()) {
+ found.add(resources.nextElement());
+ }
+ }
+ catch (IOException unreadable) {
+ return found;
+ }
+ return found;
+ }
+
+ /**
+ * Reads the settings the build states for this compilation. Only the
project being compiled
+ * contributes them, so the first one found wins rather than several being
merged.
+ */
+ private static Properties readSettings(ClassLoader loader) {
+ URL url = loader.getResource(SETTINGS_LOCATION);
+ if (url == null) {
+ return new Properties();
+ }
+ Properties settings = read(url);
+ return settings != null ? settings : new Properties();
+ }
+
+ private static Set<URL> listDescriptors(ClassLoader loader) {
Review Comment:
Fixed — descriptor URLs are resolved against the manifest that names them
(`new URL(manifest, className + ".properties")`) instead of asking the loader,
so it's O(taglibs) rather than O(taglibs × classpath). The multi-jar merge
cases in `TagLibraryIndexSpec` cover the jar-URL resolution.
##########
grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/index/TagLibraryIndexEntry.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.taglib.index;
+
+/**
+ * One tag recorded in the {@link TagLibraryIndex} at compile time.
+ *
+ * @param namespace the tag library namespace the tag is reachable through
+ * @param tagName the tag name within that namespace
+ * @param tagLibraryClassName the binary name of the tag library declaring the
tag
+ * @param kind how the tag is implemented, which decides whether a call to it
can be resolved
+ * @param acceptsBody whether the tag can be called with a body
+ * @since 8.0.0
+ */
+public record TagLibraryIndexEntry(String namespace, String tagName, String
tagLibraryClassName,
Review Comment:
`isMethod()` doesn't exist — the accessor is `isBindable()`. But the point
underneath was right and worth more than the name: nothing branches on `Kind`,
and the javadoc claimed it "decides whether a call to it can be resolved",
which is false since both forms are dispatched by name. Corrected, and `Kind`
now states why it's recorded with no consumer yet.
`FORMAT_VERSION` is back to 1. On the rest of the dead API: `findTagNames`,
`getIncompleteNamespaces` and `getAmbiguousTagNames` are gone (the last
restates `isAmbiguous`, which production uses). I kept `getTagNamesForClass`
and `isClassDescribed` — spec-only, but each states an invariant nothing else
does. Say if you want them gone too.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/TagLibraryIndexFiles.groovy:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.gradle.plugin.views.gsp
+
+import java.nio.charset.StandardCharsets
+
+import groovy.transform.CompileStatic
+
+/**
+ * The files the tag library index is made of, as the build writes them.
+ *
+ * <p>Written here rather than by the forked generator because they say what
the build asked for
+ * rather than what the sources declare, and because they have to be written
even for a project with
+ * no tag libraries of its own.
+ *
+ * @since 8.0.0
+ */
+@CompileStatic
+final class TagLibraryIndexFiles {
+
+ /**
Review Comment:
The duplication has to stay — the generator forks against the project's
compile classpath precisely because the plugin doesn't have the framework on
its own — but the keys are now constants rather than literals, and both sides
are pinned: `TagLibraryIndexFilesSpec` here and a matching assertion in
`TagLibraryIndexSpec` there, so renaming either without the other fails a test.
--
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]
