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

SLICE-128 making SliceLookUpTag type attribute value String #109

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ No more business logic in your view (JSP, Sightly scripts) - business logic's pl

**JSPs made clean and tidy** - no more these ugly scriptlets.
```jsp
<slice:lookup var="model" type="<%=com.example.components.text.TextModel%>" />
<slice:lookup var="model" type="com.example.components.text.TextModel" />
<p>${model.text}</p>
```

Expand Down Expand Up @@ -220,7 +220,7 @@ Add dependencies to your POM file:
<dependency>
<groupId>com.cognifide.slice</groupId>
<artifactId>slice-core-api</artifactId>
<version>4.3.0</version>
<version>5.0.0</version>
</dependency>
<dependency>
<groupId>com.cognifide.slice</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public class SliceLookupTag extends SimpleTagSupport {

private String appName; // auto-detected when null

private Class<?> type;
private String type;

private void clean() {
type = null;
Expand All @@ -49,14 +49,17 @@ public void doTag() throws JspException {
}

final PageContext pageContext = (PageContext) getJspContext();
final Object model = SliceTagUtils.getFromCurrentPath(pageContext, type, appName);
final Class classObject = SliceTagUtils.getClassFromType(pageContext, type);
final Object model = SliceTagUtils.getFromCurrentPath(pageContext, classObject, appName);
pageContext.setAttribute(var, model, PageContext.PAGE_SCOPE);
} catch (ClassNotFoundException cause) {
throw new JspTagException("Could not get class for " + type);
} finally {
clean();
}
}

public void setType(Class<?> type) {
public void setType(String type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the signature of this method forces you to export the package in version 5.0.0. We're not quite there to release Slice 5. Maybe we can add just new method - the should allow this package to be in version 4.4.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmajchrzak i'm not able to overload setType with String argument here. I think overloading is not supported in tag lib. do you want me to create a new attribute altogether ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep this method with changed signature, then this code will be released in Slice 5. I'm not sure about overloading in taglib neither - this needs checking. To keep backward compatibility we could also think about introducing a new parameter, but I'd definitely prefer overloading if it's possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've did some searching and trials and errors with code as well, so far what I've got is overloading is not possible for taglibs, also mentioned on this blog.

I've also did a POC on having a separate 'name' attribute. It works for sure, but is not without it's own repercussions. What if both type and name attribute are used(and say for an edge case, both have different class values), which one should be preferred is the mystery?

Leaving it up to you, should i keep looking for a way to overload the taglib attribute setter or go with a new attribute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd double check the overloading - the blog post you referenced is mentioning JavaBeans not taglibs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a look at this documentation
Setting Deferred Value Attributes and Deferred Method Attributes section in particular.

It mentions that i can have __setAttr(Object obj) __ and then check instanceOf obj.

I can do a test run of this change, but firstly what are your views on this change ?
Will the major version of Slice needs to be incremented, if i update only the setter signature and not the actual attribute type ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately yes, because the plugin which controls the version number looks at the method signatures. From my point of view, if we can't keep the current setter method, I'd postpone the implementation until the Slice 5, which is allowed to be break the API.

this.type = type;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,35 @@ public static <T> T getFromCurrentPath(final PageContext pageContext, final Clas
return getFromCurrentPath(request, injectorsRepository, requestContextProvider, type, appName);
}

/**
* A helper method that returns the {@link Class} object, resolving it via it's Canonical Name.
*
* @param pageContext allows to access request context
* @param type canonical name of the modal object, whose {@link Class} object needs to be returned
* @return {@link Class} object pertaining to {@code type} as it's canonical name
* @throws ClassNotFoundException if the class was not found
*/
public static Class<?> getClassFromType(final PageContext pageContext, final String type) throws ClassNotFoundException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmajchrzak I've also refactored this method to use ModelProvider class to get it's Class object.
Please have a look at this one as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. By introducing of this method you're fetching the model twice during tag execution. Refactor it please, so that modelProvider#get(String, Resource) is used but model is read only once.

final SlingHttpServletRequest request = SliceTagUtils.slingRequestFrom(pageContext);
final InjectorsRepository injectorsRepository = SliceTagUtils.injectorsRepositoryFrom(pageContext);

final String injectorName = getInjectorName(request, null, injectorsRepository);

final InjectorWithContext injector = injectorsRepository.getInjector(injectorName);
if (injector == null) {
throw new IllegalStateException("Guice injector not found: " + injectorName);
}
injector.pushContextProvider(contextProviderFrom(pageContext));

final ModelProvider modelProvider = injector.getInstance(ModelProvider.class);

try {
return modelProvider.get(type, request.getResource()).getClass();
} finally {
injector.popContextProvider();
}
}

/**
* A helper method that returns a model of the Sling resource related to given request
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* limitations under the License.
* #L%
*/
@Version("4.3.0")
@Version("5.0.0")
package com.cognifide.slice.api.tag;

import aQute.bnd.annotation.Version;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*-
* #%L
* Slice - Core Tests
* %%
* Copyright (C) 2012 Cognifide Limited
* %%
* Licensed 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
*
* http://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.
* #L%
*/

package com.cognifide.slice.api.tag

import com.cognifide.slice.test.setup.BaseSetup
import org.apache.sling.api.scripting.SlingBindings
import org.apache.sling.api.scripting.SlingScriptHelper
import org.apache.sling.commons.classloader.DynamicClassLoaderManager
import org.junit.Assert

import javax.servlet.ServletRequest
import javax.servlet.jsp.PageContext
import java.lang.reflect.Method

/**
* @author Shashi Bhushan
* Date: 23.08.16
*/
class SliceTagUtilsTest extends BaseSetup {

def "Get Class object, given the String type"() {

given:
final PageContext pageContext = Mock(PageContext){
getRequest()>> Mock(ServletRequest){
getAttribute(_ as String) >> Mock(SlingBindings) {
getSling() >> Mock(SlingScriptHelper) {
getService(_ as Class) >> Mock(DynamicClassLoaderManager) {
getDynamicClassLoader() >> Mock(ClassLoader) {
loadClass(_ as String) >> Class.forName(classList.className)
}
}
}
}
}
}

when:
Class classObject = SliceTagUtils.getClassFromType(pageContext, classList.className);

then: "classObject should not be null and should be equal to expected Class"
Assert.assertNotNull(classObject)
Assert.assertEquals(classList.classObject, classObject);

where:
classList << [
[
className : "com.cognifide.slice.api.tag.SliceTagUtils",
classObject : SliceTagUtils.class
]
]
}
}