Few remarks about 1.2.0 version of Shrinkwrap Descriptors

[bmajsak]

Hi guys,

First of all I would like to say that I really like the new approach with generating descriptors straight from XSDs. It saves us from quite tedious work and we can focus on important things. While working on SHRINKDESC-86 I looked at tests ported from POC and I would like to share my findings.

1. API differences

I've noticed that with the new, generated descriptors (which pretty cool idea by the way) we sacrificed some convenient behaviour from the previous version, such as:

• Mutually exclusive properties are not taken into consideration - for instance jta/non-jta datasource. If we will stick with XSD-based generation I think we should consider extending metadata to cover such cases. If you think it's reasonable to have it supported I can create JIRA issue. Currently failing tests from POC are annotated with

  {code}@Ignore("Missing in the new API"){code}

• There is no easy way to replace property with the given name (or remove it by name) as it was with the previous version by simply chaining

  {code}create().property(name, name2).property(name, name){code}

  methods we now use following construct:

{code}
Descriptors.create(PersistenceDescriptor.class)
                 .getOrCreatePersistenceUnit().name(name)
                 .getOrCreateProperties()
                 .createProperty().name(name).value(name2).up()
                 .createProperty().name(name).value(name).up().up().up().exportAsString();
{code}

In general I find it a little bit cumbersome sometimes.

• We lost vendor-specific descriptors such as Hibernate or Eclipse Link. However I don't know how important it is.

• Some things which were enums before (like SharedCacheMode) now are simply strings.

• Dealing with descriptors might sometimes look quite awkward for newcomers, for instance:

       {code}List<Property<Properties<PersistenceUnit<PersistenceDescriptor>>>> properties = def.getOrCreateProperties().getAllProperty();{code}

I know that it models type-safe tree traversal but maybe we could think about less verbose way to achieve the same thing? Also from type semantic perspective I'm not convinced if it's the way to go.

2. Other small things

• All generated enums are underscored, for instance

  {code}PersistenceUnitTransactionType._JTA{code}

• Formatting of generated classes is not always compliant with JBoss Formatter

• I also found some copied javadoc comments which were not relevant to the code actually. Maybe it's worth to review XSLT transformations?

[rbattenfeld]

Hi Bartosz

Thanks for your suggestions. I remember to had similar discussions with Aslak. Yes, it is true, the API has changed. I asked Andrew once about backwards compability. There is no such requirement. The old API was not published as far as I know.

In some way, I see your and also Aslak's opinion. If you just focus on one particular situation for a specific descriptor, you may think can be simplified. But from a generation point of view, I can not see a way to generate many, many descriptors and each of them have specialized, convenient interfaces. It is a question of providing a few, hand made descriptors, which are potentially incomplete, or to provide many descriptors covering the specification with all the complex details.

Andrew told me also that it is not sure that the XSD approach is surviving. This is open source and it is absolutely ok to me to replace this with something which provides better finctionalitities, or is better accepted. I have no problem with that. In any cases, I truly believe that only a generation approach provides complete and stable descriptors.

Of course, we can change the API even for this XSD approach to any other form but it should be something consistent covering all xsd's.

The reason for the underscore in enums is that some of the enums are starting with a number like 1.x which is does not compile. So I simply added a underscore. It is possible to check the first character and only if that character is a number, a underscore is added.

If think the basic question is: what is the main purpose of this project? As a user of Arquillian and Shrinkwarp, I just want to produce descriptors on the fly, which is provided. I learned during my long time working as software guy (yes, I am a old guy :-) ), there is no 100% solution.

[bmajsak]

Hi Ralf,

I'm afraid there is a small misunderstanding

The point I was trying to make wasn't actually about backward compatibility in the API but rather about providing easy way of creating or manipulating the descriptor. I was just wondering if it would be possible to provide some notion of metadata for the specific descriptor and let XSLT (or any other generation strategy) deal with that while generating classes, saying for instance that *non-jta* and *jta* data source are mutually exclusive elements - and based on that implement the replacement logic in the fluent interface. I'm only having in mind end-user convenience.

What do you think?

Cheers,

Bartosz

[rbattenfeld]

Hi Bartosz

Now, I think got your point:-) This sounds very interesting to me. We could introduce annotations and the generated code simply delegates this part to a known class or a configured classes (like the node handling), maybe per descriptor and/or global for all descriptors.

Cool suggestion! 

[rbattenfeld]

Hi

There are some API enhancements I have in mind:

Validation: Each descriptor provides a method called validate(). This call executes an validation against the particular schema file and throws an exception if the validation failes. There are three options I see:

1. the schemas are part of the descriptor jar
2. it is resolved via namespace
3. the user has to provide it.

I think, this piece of code can be generated via the new plugin.

Convenience API: I believe that the convienence API should not be generated individually per descriptor. But there are some options I see:

1. The interface descriptor is extended with a method called strict(boolean b). If set the false, then the generated descriptor is passed to dedicated class or serie of classes that do some convience stuff.
2. Dedicated annotations per descriptor. Here we put all the infos into the annotation. The user is just calling Descriptor.create(PersistenceDescriptor.class) and a dedicated class analysing the annotations will then populate the generated descriptor with the defined info's. This could be a solution for the Eclipse/Hibernate vendor specific descriptors.

General API improvements:

• Mixed elements can be implemented better:-) Currently, the API is .createConfigProperty().name("name1").text("config-property0") but better is: .createConfigProperty("config-property0").name("name1")

This are some ideas from my side

Ralf

[alrubinger]

Ralf Battenfeld wrote:

Validation: Each descriptor provides a method called validate(). This call executes an validation against the particular schema file and throws an exception if the validation failes. There are three options I see:

1. the schemas are part of the descriptor jar
2. it is resolved via namespace
3. the user has to provide it.

I actually think we should move Validation as a concern outside the object model of the metadata entirely; it's a lesson I've learned from jboss-metadata which IMO attempts to do too much all at once.  So I'd instead create a Validator for each Descriptor type, and leave the Descriptors to simply be value objects.  That said, I'm not sure how much of the validation logic we can generate from the XSDs alone (and certainly from DTD-based descriptors).  We might need some extra mechanism to supply the code generator which creates the Validators to take in some optional metadata where we can place additional constraints.

Ralf Battenfeld wrote:

Convenience API: I believe that the convienence API should not be generated individually per descriptor. But there are some options I see:

1. The interface descriptor is extended with a method called strict(boolean b). If set the false, then the generated descriptor is passed to dedicated class or serie of classes that do some convience stuff.
2. Dedicated annotations per descriptor. Here we put all the infos into the annotation. The user is just calling Descriptor.create(PersistenceDescriptor.class) and a dedicated class analysing the annotations will then populate the generated descriptor with the defined info's. This could be a solution for the Eclipse/Hibernate vendor specific descriptors.

Do we really need something more convenient than a typesafe factory method:

  WebAppDescriptor d = Descriptors.create(WebAppDescriptor.class);

...?  or what am I missing with your annotations example?  Maybe see some psedocode?

General API improvements:

• Mixed elements can be implemented better:-) Currently, the API is .createConfigProperty().name("name1").text("config-property0") but better is: .createConfigProperty("config-property0").name("name1")

Hmm, I could see something along those lines.  But wouldn't the argument passed into the createConfigProperty method be the name, not the text?  Or take two arguments, one for each name and text?

Another thing I *really* want to do sooner than later is separate out the read-only vs. mutable views into two interfaces.  ie. WebAppDescriptor and WebAppMutableDescriptor.  That's https://issues.jboss.org/browse/SHRINKDESC-21.

S,

ALR

[rbattenfeld]

Thanks Andrew for your comments. I will come back to the first 3 comments soon.

For the last one, the read-only view, this we can achieve more or less easily by little change of the XSLT step. If it is agreed to have two interface like WebAppDescriptor versus WebAppMutableDescriptor then we can have this in one or two weeks for all existing descriptors.

Regards,

Ralf

[rbattenfeld]

Hi Andrew

Here are the rest of the comments:

Validation:

I actually think we should move Validation as a concern outside the object model of the metadata entirely; it's a lesson I've learned from jboss-metadata which IMO attempts to do too much all at once.  So I'd instead create a Validator for each Descriptor type, and leave the Descriptors to simply be value objects.  That said, I'm not sure how much of the validation logic we can generate from the XSDs alone (and certainly from DTD-based descriptors).  We might need some extra mechanism to supply the code generator which creates the Validators to take in some optional metadata where we can place additional constraints.

I agree. It is new interface in the api base module which just validates a given file, stream, string against the schema. The code for this is standard DOM validation:

// parse an XML document into a DOM tree

    DocumentBuilder parser = DocumentBuilderFactory.newInstance().newDocumentBuilder();

    Document document = parser.parse(new File("instance.xml"));

    // create a SchemaFactory capable of understanding WXS schemas

    SchemaFactory factory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI);

    // load a WXS schema, represented by a Schema instance

    Source schemaFile = new StreamSource(new File("mySchema.xsd"));

    Schema schema = factory.newSchema(schemaFile);

    // create a Validator instance, which can be used to validate an instance document

    Validator validator = schema.newValidator();

    // validate the DOM tree

    try {

        validator.validate(new DOMSource(document));

    } catch (SAXException e) {

        // instance document is invalid!

    }

Convenience API:

Do we really need something more convenient than a typesafe factory method:

  WebAppDescriptor d = Descriptors.create(WebAppDescriptor.class);

...?  or what am I missing with your annotations example?  Maybe see some psedocode?

This is related to Aslak's and Bartosz's suggestions. The old descriptors (I don't know how to called it else:-) ) provide so called convenience methods that automatically fill other fields like servlet -> servlet-mapping or filter -> filter-mapping. I also see this outside of the generated descriptors as an optional task. The interface could be called DescriptorOptimazer that has a serie of filter classes and passes the generated descriptor to all filters. One of the filters is handling webDescriptors and just looks for servlets and creates servlet-mappings. Something like this. This we could also trigger by a dedicated annotation.

S.

Ralf

[dan.j.allen]

First, I want to say that I appreciate the work you are doing to make ShrinkWrap Descriptors robust and complete, Ralf. Prior to your involvement, descriptors was stuck in prototype land. I'm looking forward to when it becomes a cornerstone of Arquillian tests.

I have some feedback that I'd like to share to help us find the right path.

My first impression of the 1.2.0 API was nearly identical to what Bartosz shared. It was the first time I had used the API since the original prototype and I just didn't feel good about how it was coming out after the refactoring.

Here are three examples with the before and after, which I'll use to point out a few shortcomings.

Before #1

.setWebXML(new StringAsset(Descriptors.create(WebAppDescriptor.class)
    .envEntry("name", String.class, "infinispan.xml").exportAsString()));

After #1

.setWebXML(new StringAsset(Descriptors.create(WebAppDescriptor.class)
    .getOrCreateEnvEntry().envEntryName("name").envEntryType("java.lang.String").envEntryValue("infinispan.xml").up()
    .exportAsString());

Before #2

.setWebXML(new StringAsset(Descriptors.create(WebAppDescriptor.class)
    .servlet(EchoServlet.class, EchoServlet.URL_PATTERN).exportAsString()));

After #2

.setWebXML(new StringAsset(Descriptors.create(WebAppDescriptor.class)
    .createServlet()
        .servletName(EchoServlet.class.getSimpleName())
        .servletClass(EchoServlet.class.getName()).up()
    .createServletMapping()
        .servletName(EchoServlet.class.getSimpleName())
        .urlPattern(EchoServlet.URL_PATTERN).up()
    .exportAsString()));

Before #3

.addAsWebInfResource(new StringAsset(Descriptors.create(BeansDescriptor.class)
    .alternativeClass(MockPaymentProcessor.class).exportAsString())

After #3

.addAsWebInfResource(new StringAsset(Descriptors.create(BeansDescriptor.class)
    .createAlternatives().clazz(MockPaymentProcessor.class.getName()).up()
    .exportAsString())

up() method:

I want to start off by saying that I don't like the up() method at all. I think that's an indication of a fluent API that isn't yet all there, and it's very awkward (see #1, #2 and #3). The consumer of the API should not be aware of the internal traversal that is needed to step out of working on the current node. If you begin an operation on another node, the nesting level change should be implicit.

If there is absolutely no way to avoid this method, then I propose we at least give it a friendlier name, such as closeElement(), endElement() or simply close().

package names:

We've learned from naming the containers that version numbers should never be condensed in a package or artifact name since it causes ambiguity and it's not future proof. Here's an example of one of these packages today:

org.jboss.shrinkwrap.descriptor.api.beans10.BeansDescriptor

This package should instead be:

org.jboss.shrinkwrap.descriptor.api.beans_1_0.BeansDescriptor

It's longer, but it's clear and safe. The consumer isn't left stratching their heading thinking 10?

class names as strings:

Whenever a full-qualified class name is needed, the method should be overloaded to accept either a class reference or a full-qualified class name string, consistent w/ ShrinkWrap archives. In SWD 1.2.0, a string is the only option. Not only do we lose the chance at type safety, it also increases code verbosity. (see #2 and #3)

The same goes for parameters that are types. Even if a "type" could be a generic reference, if a class reference is possible, SWD should have a method that accomodates that parameter format. (see envEntryType() in #1).

There's a similar problem with enums as values. I think when a value is accepted, the method signature should be changed to Object and then toString() called on it before accepting the value. For example, this change would accomodate: paramValue(ProjectStage.Development).

getOrCreate*

The getOrCreate* method sounds like a method that's not sure what it wants to do. To some, this combination of behavior is a code smell. I get the intent, but we can make this look a lot cleaner. Here is one idea:

contextParam().paramName(ProjectStage.PROJECT_STAGE_PARAM_NAME).paramValue(ProjectStage.Development.name()).save()

The save() method (or alternate name) at the end of the chain would try to find an existing parameter with the same key (meaning however an element is uniquely defined). If an element is found, it updates that element. If the element is not found, a new one is created and inserted. We've also changed the purpose of the getOrCreate* method to merely descend us into the context of the element (lazy traversal).

If two methods are necessary, we can have "save()" /  "define()" (add but fails if there is an existing element) and "update()" (which adds or creates)

Associated elements

I know this one is tricky, but you can see in #2 that the servlet and servlet mappings now have to be lined up on the servlet name as the key. This might be a valid case when we need to "giftwrap" the ShrinkWrap Descriptor API to offer a convenience method that treats the definition of these two elements as a single chain (the fact that they are represented as two elements is really just an implemention detail (or quirk).

exportAsString()

Although it's always been around, the need to exportAsString(), and even StringAsset() seems like a leaky abstraction. I'd really like to see support in either ShrinkWrap Archives, SWD or an integration library that can convert a Descriptor into an Asset. To be honest, I don't see why we can't have:

Descriptors.create(BeanDescriptor.class).asAsset()

(maybe move Asset to ShrinkWrap Core if necessary) (and split off ShrinkWrap Archives).

Wrap-up

I know it's not easy to go from an XSD to the type of customizations I'm talking about. However, if we have transformers at generation time that can but some of this logic in place for all descriptors, or can offer consolidated functionality for things like defining a Servlet and Servlet mapping in a single logical element.

Let's not forget the consumer of the API. I think there is plenty of room to offer them something friendlier than what we have in 1.2.0. We'll get it!

[rbattenfeld]

Hi Dan

Thank you for your comments. I truly agree on most of your comments. No doubt on this. I think, there was a big misunderstanding in the first comment made by me. We worked hard on the technical aspect of the descriptor project. We have now a maven plugin that allows much easier to create new descriptors, including DTD support und so on...

I totally agree that we have to focus now on the API and to make it more user friendly. Did you read JIRA SHRINKDESC-119? There was a proposal made by Aslak quite a while ago addressing some of problems you commented out. Personally, I don't think that this 'type' approach is a step forward. But I am Swiss, a democratic decision is always fine for me.

In the meanwhile, I will think what we can do out of the generation process itself. There are for sure possiblities on this level.

S,

Ralf

[dan.j.allen]

That's good to hear Ralf. I don't want to be overly critical of a work in progress, so like a great piece of sculpture, I won't judge it before all the excess is chiseled away

Looking back, I realized that my class parameter is type-safe after all, since I call the getName() method on the class object. So the real problem there is that it's just verbose. I'm not convinced that SHRINKWARP-119 actually solves that problem. It seems just as verbose as using getName(). I do understand that the proposal is working to address the coorleation and annotation scanning problem, so it has another purpose. It may even bee too verbose for that purpose.

For sure, let's see where we can get with the generation process. I've done a very similar project in the past and was able to demonstrate that there is a lot of flexibility in it.