Sling
  1. Sling
  2. SLING-2477

Configuration via sling:OsgiConfig nodes does not support all types

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: JCR Installer 3.1.2
    • Fix Version/s: None
    • Component/s: Installer
    • Labels:
      None

      Description

      Most notably, the common "service.ranking" needs to be an Integer, while the jcr property mapping only allows for "Long" types at the moment. The problem is that JCR has a smaller set of property types than the OSGi config admin (JCR: String, Boolean, Long, Double, Decimal; OSGi: String, Boolean, Long, Integer, Float, Double, and probably more differences...).

      Similarly to properties files (which do it in the value like 'service.ranking=I"-10000"' with I=Integer), there must be a way to explicitly specify the type regardless of the JCR type. For example, encoding it in the property name like "service.ranking

      {int}

      ".

        Issue Links

          Activity

          Hide
          Felix Meschberger added a comment -

          I support the general idea.

          Yet, I suggest we use the same encoding for string values as we do the configuration properties files supported by JCR Install.

          Clarification: JCR typing and Configuration Admin typing are overlapping. Neither is a subset of the other.

          Show
          Felix Meschberger added a comment - I support the general idea. Yet, I suggest we use the same encoding for string values as we do the configuration properties files supported by JCR Install. Clarification: JCR typing and Configuration Admin typing are overlapping. Neither is a subset of the other.
          Show
          Alexander Klimetschek added a comment - Added a FAQ entry for the workaround on this: https://cwiki.apache.org/confluence/display/SLING/FAQ#FAQ-Howtochangetheservice.rankingofaservicethroughconfiguration%3F
          Hide
          Ian Boston added a comment -

          Picking this up and parsing String property values to check if they are formatted as OSGi property files. Below is a set of test patterns in the form
          test, typeprefix, extracted value, valid typeprefix, java object value.

          One thing that jumps out is that any configuration that matches the pattern /^([TIiLlFfDdSsXxCcBb])"(.*)"/ will get converted, which may impact Sling users.

          I assume that this is a small price to pay ? (and unlikely to cause problems). WDYT?

          private static final Object[][] MATCH_PATTERNS_CONVERTER = {

          { "T\"testing\"","T","testing",true,"testing" }

          ,

          { "I\"101\"","I", "101",true, new Integer(101) }

          ,

          { "i\"101\"","i","101", true, (int) 101 }

          ,

          { "L\"1010\"","L","1010", true, new Long(1010) }

          ,

          { "l\"1010\"","l","1010", true, 1010L }

          ,

          { "F\"101.01\"","F","101.01", true, new Float(101.01) }

          ,

          { "f\"101.01\"","f","101.01", true, 101.01F }

          ,

          { "D\"101.02\"","D","101.02", true, new Double(101.02) }

          ,

          { "d\"101.03\"","d","101.03", true, 101.03D }

          ,

          { "X\"0x34\"","X","0x34", true, new Byte((byte)0x34) }

          ,

          { "x\"0x35\"","x","0x35", true, (byte)0x35 }

          ,

          { "S\"34\"","S","34", true, new Short((short) 34) }

          ,

          { "s\"35\"","s","35", true, (short)35 }

          ,

          { "C\"a\"","C","a", true, new Character('a') }

          ,

          { "c\"b\"","c","b", true, 'b'}

          ,

          { "B\"true\"","B","true", true, new Boolean(true) }

          ,

          { "b\"false\"","b","false", true, false }

          ,
          };

          Show
          Ian Boston added a comment - Picking this up and parsing String property values to check if they are formatted as OSGi property files. Below is a set of test patterns in the form test, typeprefix, extracted value, valid typeprefix, java object value. One thing that jumps out is that any configuration that matches the pattern /^( [TIiLlFfDdSsXxCcBb] )"(.*)"/ will get converted, which may impact Sling users. I assume that this is a small price to pay ? (and unlikely to cause problems). WDYT? private static final Object[][] MATCH_PATTERNS_CONVERTER = { { "T\"testing\"","T","testing",true,"testing" } , { "I\"101\"","I", "101",true, new Integer(101) } , { "i\"101\"","i","101", true, (int) 101 } , { "L\"1010\"","L","1010", true, new Long(1010) } , { "l\"1010\"","l","1010", true, 1010L } , { "F\"101.01\"","F","101.01", true, new Float(101.01) } , { "f\"101.01\"","f","101.01", true, 101.01F } , { "D\"101.02\"","D","101.02", true, new Double(101.02) } , { "d\"101.03\"","d","101.03", true, 101.03D } , { "X\"0x34\"","X","0x34", true, new Byte((byte)0x34) } , { "x\"0x35\"","x","0x35", true, (byte)0x35 } , { "S\"34\"","S","34", true, new Short((short) 34) } , { "s\"35\"","s","35", true, (short)35 } , { "C\"a\"","C","a", true, new Character('a') } , { "c\"b\"","c","b", true, 'b'} , { "B\"true\"","B","true", true, new Boolean(true) } , { "b\"false\"","b","false", true, false } , };
          Hide
          Felix Meschberger added a comment -

          I wonder what the use-case/advantage of using quotes would be. I.e. why is I"1000" better than I1000 ?

          Show
          Felix Meschberger added a comment - I wonder what the use-case/advantage of using quotes would be. I.e. why is I"1000" better than I1000 ?
          Hide
          Ian Boston added a comment -

          Not sure, I thought thats what the existing properties format was as described in the description above. Might have missunderstood.

          However, there is an advantage.

          I"1000" matches far fewer potential values than
          /^([TIiLlFfDdSsXxCcBb])(.*)/

          This could be narrowed down with parses/regexes
          /^([IiLlSs])([+\-0-9].*)$/
          /^([DdFf])([+\-\.Ee0-9].*)$/
          /^([Xx])([x0-9].*)$/
          /^([Cc])(.)$/
          /^([Bb])([tfyn]|true|false)$/
          BTW, those are not tested or complete, just for illustration purposes.

          But,
          /^T(.*)$/ for explicit text is going to match a lot of strings which will make migrating existing sling:OsgiConfig nodes expensive, so it seems simpler to use
          /^([TIiLlFfDdSsXxCcBb])"(.*)"/

          Show
          Ian Boston added a comment - Not sure, I thought thats what the existing properties format was as described in the description above. Might have missunderstood. However, there is an advantage. I"1000" matches far fewer potential values than /^( [TIiLlFfDdSsXxCcBb] )(.*)/ This could be narrowed down with parses/regexes /^( [IiLlSs] )( [+\-0-9] .*)$/ /^( [DdFf] )( [+\-\.Ee0-9] .*)$/ /^( [Xx] )( [x0-9] .*)$/ /^( [Cc] )(.)$/ /^( [Bb] )( [tfyn] |true|false)$/ BTW, those are not tested or complete, just for illustration purposes. But, /^T(.*)$/ for explicit text is going to match a lot of strings which will make migrating existing sling:OsgiConfig nodes expensive, so it seems simpler to use /^( [TIiLlFfDdSsXxCcBb] )"(.*)"/
          Hide
          Justin Edelson added a comment -

          Ian - while these values are obscure enough that the backwards compatibility risk is minor, I do think Alex's original suggesting of putting the type information in the property name is safer. Is that not feasible for some reason?

          Show
          Justin Edelson added a comment - Ian - while these values are obscure enough that the backwards compatibility risk is minor, I do think Alex's original suggesting of putting the type information in the property name is safer. Is that not feasible for some reason?
          Hide
          Justin Edelson added a comment -

          Regardless of the implementation, if this is fixed, SLING-1971 should be revisited to do lossless writeback to sling:OsgiConfig nodes using this syntax.

          Show
          Justin Edelson added a comment - Regardless of the implementation, if this is fixed, SLING-1971 should be revisited to do lossless writeback to sling:OsgiConfig nodes using this syntax.
          Hide
          Ian Boston added a comment -

          Either approach is possible. As you say

          {.*} has the advantage of being safer from a migration pov, assuming {.*}

          is not confused somewhere in jcr for a namespace. (I checked with curl and no problems seen)

          Putting the information in the property name has the drawback that multiple properties can exist with different types, and there is potential to confuse a novice configurer who might think they are 3 properties rather than one, not realising the significance of the

          {.*}

          eg
          service.ranking
          service.ranking

          {int}

          service.ranking

          {long}

          Would need to know which one to use and what to do. Options include:
          a) If more than one exists, throw an exception of some form.
          b) use the most specific, in the example above that might be {long}

          .

          Show
          Ian Boston added a comment - Either approach is possible. As you say {.*} has the advantage of being safer from a migration pov, assuming {.*} is not confused somewhere in jcr for a namespace. (I checked with curl and no problems seen) Putting the information in the property name has the drawback that multiple properties can exist with different types, and there is potential to confuse a novice configurer who might think they are 3 properties rather than one, not realising the significance of the {.*} eg service.ranking service.ranking {int} service.ranking {long} Would need to know which one to use and what to do. Options include: a) If more than one exists, throw an exception of some form. b) use the most specific, in the example above that might be {long} .
          Hide
          Ian Boston added a comment -

          This issue must be reviewed to ensure lossless roundtrip

          Show
          Ian Boston added a comment - This issue must be reviewed to ensure lossless roundtrip
          Hide
          Carsten Ziegeler added a comment -

          I still think this is something that has to be fixed in the JCR spec, although the name is "Java" content repository it does not support the basic types....but well, definitely not something we can fix here.
          I think, Ian's first approach to use the " like I"100" seems to cause the least problems. Having a separate property for each property with the type is too confusing.
          Now, in addition we only need the type information if the type is not supported by JCR, so no extra information required for strings, longs or booleans - we need it for float, int, byte, char, and short.
          So we either store these as string properties in the format [TYPE_INFO]"[value]" - or the other solution I see is we store a single additional property containing the type information. Like a string array with values in the format [name]:[type]. The value itself is then stored as a string.
          However, having a single property is easier to handle and maintain.

          Once we have this, I agree we should create a new issue to improve SLING-1971 and support write back of both, config nodes and config files - depending on their previous format.

          Show
          Carsten Ziegeler added a comment - I still think this is something that has to be fixed in the JCR spec, although the name is "Java" content repository it does not support the basic types....but well, definitely not something we can fix here. I think, Ian's first approach to use the " like I"100" seems to cause the least problems. Having a separate property for each property with the type is too confusing. Now, in addition we only need the type information if the type is not supported by JCR, so no extra information required for strings, longs or booleans - we need it for float, int, byte, char, and short. So we either store these as string properties in the format [TYPE_INFO] " [value] " - or the other solution I see is we store a single additional property containing the type information. Like a string array with values in the format [name] : [type] . The value itself is then stored as a string. However, having a single property is easier to handle and maintain. Once we have this, I agree we should create a new issue to improve SLING-1971 and support write back of both, config nodes and config files - depending on their previous format.
          Hide
          Alexander Klimetschek added a comment - - edited

          I suggested the service.ranking

          {int} syntax mostly because it's more intuitive and because it would still use the JCR types as much as possible, instead of going back to always using a string and loosing the usefulness of JCR types (what the I"1000" notation implies).

          It would use the "closest" JCR property type:
          - byte => LONG
          - short => LONG
          - int => LONG
          - float => DOUBLE
          - char => STRING

          If values exceed their interval, we can either take a best effort (first char of a string or maximum byte/short/int/float value), ignore them or throw an error. Don't know what's best.

          For completeness, the ones hat map 1:1:
          - string => STRING
          - long => LONG
          - double => DOUBLE
          - bool => BOOLEAN

          The types that are in JCR, but not in Java/OSGi configs, can be ignored. Maybe the implementation uses the JCR property toString() mapping and sees them as string properties (if that already happens in the current version).

          If multiple properties are present (service.ranking, service.ranking{int}

          , service.ranking

          {long}

          ) one could for example use the first valid entry. It would be a very obvious mistake by the user/dev if he adds multiple of those properties, so nothing that we need to do much about.

          BTW, I think it's good that JCR simplifies primitive types instead of supporting all Java primitives. I have never seen a use case for byte, short, char or int limitations for persistence reasons.

          Show
          Alexander Klimetschek added a comment - - edited I suggested the service.ranking {int} syntax mostly because it's more intuitive and because it would still use the JCR types as much as possible, instead of going back to always using a string and loosing the usefulness of JCR types (what the I"1000" notation implies). It would use the "closest" JCR property type: - byte => LONG - short => LONG - int => LONG - float => DOUBLE - char => STRING If values exceed their interval, we can either take a best effort (first char of a string or maximum byte/short/int/float value), ignore them or throw an error. Don't know what's best. For completeness, the ones hat map 1:1: - string => STRING - long => LONG - double => DOUBLE - bool => BOOLEAN The types that are in JCR, but not in Java/OSGi configs, can be ignored. Maybe the implementation uses the JCR property toString() mapping and sees them as string properties (if that already happens in the current version). If multiple properties are present (service.ranking, service.ranking{int} , service.ranking {long} ) one could for example use the first valid entry. It would be a very obvious mistake by the user/dev if he adds multiple of those properties, so nothing that we need to do much about. BTW, I think it's good that JCR simplifies primitive types instead of supporting all Java primitives. I have never seen a use case for byte, short, char or int limitations for persistence reasons.
          Hide
          Carsten Ziegeler added a comment -

          I think we should keep the potential for user errors to a minimum, thereforw I would not go with something which provides the possiblity to have several definitions for the same property at the same time.

          Show
          Carsten Ziegeler added a comment - I think we should keep the potential for user errors to a minimum, thereforw I would not go with something which provides the possiblity to have several definitions for the same property at the same time.
          Hide
          Alexander Klimetschek added a comment -

          When do you think would someone do this and expect a useful outcome?

          service.ranking
          service.ranking

          {int}

          service.ranking

          {short}

          This would be on the same level as typos, which we also cannot prevent. {*} is a very obvious typing hint that you can learn from simply by looking at other configs that use it. With the cryptic "I" syntax, it's not obvious and can lead to more errors IMHO (such as not using the right type, as in the service.ranking case).

          Show
          Alexander Klimetschek added a comment - When do you think would someone do this and expect a useful outcome? service.ranking service.ranking {int} service.ranking {short} This would be on the same level as typos, which we also cannot prevent. {*} is a very obvious typing hint that you can learn from simply by looking at other configs that use it. With the cryptic "I" syntax, it's not obvious and can lead to more errors IMHO (such as not using the right type, as in the service.ranking case).
          Hide
          Carsten Ziegeler added a comment -

          I've seen a lot of strange things, so be assured someone will do this - and if we can prevent doing this by picking a different solution we should go this route. Or throw an exception and not use the configuration at all, but applying some best effort approach is causing for trouble.

          Show
          Carsten Ziegeler added a comment - I've seen a lot of strange things, so be assured someone will do this - and if we can prevent doing this by picking a different solution we should go this route. Or throw an exception and not use the configuration at all, but applying some best effort approach is causing for trouble.
          Hide
          Ian Boston added a comment -

          To assist in decision making:

          Attached a patch of the solution based on I"1000" approach with basic unit test coverage.

          It would be relatively easy to adapt to other approaches once a decision is made.

          Show
          Ian Boston added a comment - To assist in decision making: Attached a patch of the solution based on I"1000" approach with basic unit test coverage. It would be relatively easy to adapt to other approaches once a decision is made.
          Hide
          Ian Boston added a comment -

          Created a new issue to review SLING-1971 post fixing this issue.

          Show
          Ian Boston added a comment - Created a new issue to review SLING-1971 post fixing this issue.
          Hide
          Ian Boston added a comment -

          I think the consensus was that the I"*" approach was likely to create the fewest problems. Unless anyone objects I will apply the patch as posted in 24h and move on to addressing the write back issue SLING-2752

          Show
          Ian Boston added a comment - I think the consensus was that the I"*" approach was likely to create the fewest problems. Unless anyone objects I will apply the patch as posted in 24h and move on to addressing the write back issue SLING-2752
          Hide
          Alexander Klimetschek added a comment -

          I disagree about consensus The problem with a string based value is that you no longer can use JCR's native types. And frankly, it's quite an obscure syntax. It's ok for byte codes where you want to minimize space and readability is not important, but for a configuration "file" format, usability should always win, as that leads to less errors.

          Show
          Alexander Klimetschek added a comment - I disagree about consensus The problem with a string based value is that you no longer can use JCR's native types. And frankly, it's quite an obscure syntax. It's ok for byte codes where you want to minimize space and readability is not important, but for a configuration "file" format, usability should always win, as that leads to less errors.
          Hide
          Carsten Ziegeler added a comment -

          We can't come to a solution where everyone agrees So +1 to going forward - the syntax should only be used for types which are not directly supported by JCR and then we're fine. IMHO people should not change configurations directly within the repository but rather use configuration admin and the write back feature of the installer. And if we use the same syntax for the properties as we're using for the config file, then people just need to learn one way of defining the props

          Show
          Carsten Ziegeler added a comment - We can't come to a solution where everyone agrees So +1 to going forward - the syntax should only be used for types which are not directly supported by JCR and then we're fine. IMHO people should not change configurations directly within the repository but rather use configuration admin and the write back feature of the installer. And if we use the same syntax for the properties as we're using for the config file, then people just need to learn one way of defining the props
          Hide
          Felix Meschberger added a comment -

          FIrst off: Yes, I admit that I don't like the name

          {type}

          notation. Because this stipulates the name is typed, where actually value is typed. And yes, I don't particularly like the quoting of the type"value" notation, but I can live with it.

          What is really important is, that we do not have different formats for the configuration files and the configuration properties – with the exception, that I agree to fully leverage JCR types.

          Show
          Felix Meschberger added a comment - FIrst off: Yes, I admit that I don't like the name {type} notation. Because this stipulates the name is typed, where actually value is typed. And yes, I don't particularly like the quoting of the type"value" notation, but I can live with it. What is really important is, that we do not have different formats for the configuration files and the configuration properties – with the exception, that I agree to fully leverage JCR types.
          Hide
          Alexander Klimetschek added a comment -

          @Felix: good points. Though the config file format was introduced without much discussion IIRC, so wrapping it back on the original sling:OsgiConfig way just because it is there is not a very strong argument IMO

          If there is no one else sharing my desire for a more intuitive format (I am still hoping for some feedback from outside of our small group), then at least only add the string notation for the non-JCR types. E.g. int, short, byte, float & char: "IiSsXxFf". The whole point of doing a sling:OsgiConfig in the first place (before the config file variant was added) was to use JCR's fine granular structure as much as possible and this includes types.

          Show
          Alexander Klimetschek added a comment - @Felix: good points. Though the config file format was introduced without much discussion IIRC, so wrapping it back on the original sling:OsgiConfig way just because it is there is not a very strong argument IMO If there is no one else sharing my desire for a more intuitive format (I am still hoping for some feedback from outside of our small group), then at least only add the string notation for the non-JCR types. E.g. int, short, byte, float & char: "IiSsXxFf". The whole point of doing a sling:OsgiConfig in the first place (before the config file variant was added) was to use JCR's fine granular structure as much as possible and this includes types.
          Hide
          Justin Edelson added a comment -

          I completely agree with Alex and I really don't understand the rationale for not using the name

          {type}

          notation.

          @Felix - Do you really think that anyone is going to think that foo

          {short}

          is somehow saying that the name "foo" is a short.

          @Carsten - While writeback is a useful tool, we also need to recognize that sling:OsgiConfig nodes are frequently edited by hand in an offline form, not via writeback. This is especially true in complex environments with multiple runmodes where clarity is especially important.

          The file format used by Felix's ConfigAdmin is a (AFAIK undocumented) implementation detail of Felix's ConfigAdmin and IMHO exposing it in SLING-1971 was a mistake (but a necessary mistake given that writeback was already a work in progress). It would be a shame to continue down that path for the sake of consistency.

          We should implement the approach originally suggested by Alex then deprecate and eventually remove support for .config files.

          Show
          Justin Edelson added a comment - I completely agree with Alex and I really don't understand the rationale for not using the name {type} notation. @Felix - Do you really think that anyone is going to think that foo {short} is somehow saying that the name "foo" is a short. @Carsten - While writeback is a useful tool, we also need to recognize that sling:OsgiConfig nodes are frequently edited by hand in an offline form, not via writeback. This is especially true in complex environments with multiple runmodes where clarity is especially important. The file format used by Felix's ConfigAdmin is a (AFAIK undocumented) implementation detail of Felix's ConfigAdmin and IMHO exposing it in SLING-1971 was a mistake (but a necessary mistake given that writeback was already a work in progress). It would be a shame to continue down that path for the sake of consistency. We should implement the approach originally suggested by Alex then deprecate and eventually remove support for .config files.
          Hide
          Carsten Ziegeler added a comment -

          I disagree - it's true that this is not very well documented, but that's just because it never happened and is an open todo. Supporting the config admin format is a great feature as this allows to create the configuration somewhere and simply copy this file into a repository.
          I still think that people should not edit OSGi configurations in the repository, there is zero support wrt to available configuration options, valid values or their meaning. The web console is a way better tool as this leverages the metat type information. I understand that people did directly edit inside the repo because changes through the web console were not persisted. But we fixed this long time ago.

          We can argue here forever, and imho we should go with the solution which calls for the least trouble and that is the I"" notation. If people are able to add a hello

          {int}

          property, they sure will be able to create a hello property with a value I"353". Both have an artifical syntax and one might look nicer than the other, but still you need to know what the correct syntax is, so from a practical pov there is no difference.

          Show
          Carsten Ziegeler added a comment - I disagree - it's true that this is not very well documented, but that's just because it never happened and is an open todo. Supporting the config admin format is a great feature as this allows to create the configuration somewhere and simply copy this file into a repository. I still think that people should not edit OSGi configurations in the repository, there is zero support wrt to available configuration options, valid values or their meaning. The web console is a way better tool as this leverages the metat type information. I understand that people did directly edit inside the repo because changes through the web console were not persisted. But we fixed this long time ago. We can argue here forever, and imho we should go with the solution which calls for the least trouble and that is the I"" notation. If people are able to add a hello {int} property, they sure will be able to create a hello property with a value I"353". Both have an artifical syntax and one might look nicer than the other, but still you need to know what the correct syntax is, so from a practical pov there is no difference.
          Hide
          Justin Edelson added a comment -

          @Carsten - this is perhaps OT, but configurations are never going to only be edited via the web console. If you have configuration in the config.production folder (i.e. for a production runmode), that gets edited by hand and committed to version control, not done through the production web console.

          Show
          Justin Edelson added a comment - @Carsten - this is perhaps OT, but configurations are never going to only be edited via the web console. If you have configuration in the config.production folder (i.e. for a production runmode), that gets edited by hand and committed to version control, not done through the production web console.
          Hide
          Carsten Ziegeler added a comment -

          Ok, yes, so there are use cases for direct JCR editing, use cases for editing through web console/config admin, and use cases for supporting the config admin file format in the repository

          Show
          Carsten Ziegeler added a comment - Ok, yes, so there are use cases for direct JCR editing, use cases for editing through web console/config admin, and use cases for supporting the config admin file format in the repository
          Hide
          Justin Edelson added a comment -

          I'm not sure why one would want to use the config admin file format if writeback updated sling:OsgiConfig nodes, but that's for another time...

          Show
          Justin Edelson added a comment - I'm not sure why one would want to use the config admin file format if writeback updated sling:OsgiConfig nodes, but that's for another time...
          Hide
          Tyson Norris added a comment -

          +1 for type info in the value (not the property name)
          +1 for only REQUIRING this format for values that are not supported by JCR (otherwise I don't know the upgrade implications, but it seems complicated)
          +1 for have a SINGLE format for serializing config to content - using files in some cases and not others is bad and confusing
          BTW - in case it is an open question: we MUST have a way to install configs that are not managed via the admin console, there are too many cases where app configs cannot be managed via UI

          Show
          Tyson Norris added a comment - +1 for type info in the value (not the property name) +1 for only REQUIRING this format for values that are not supported by JCR (otherwise I don't know the upgrade implications, but it seems complicated) +1 for have a SINGLE format for serializing config to content - using files in some cases and not others is bad and confusing BTW - in case it is an open question: we MUST have a way to install configs that are not managed via the admin console, there are too many cases where app configs cannot be managed via UI
          Hide
          Ian Boston added a comment -

          (apologies for saying "consensus" earlier).

          To summarise both approaches.

          name

          {type} approach:
          -------------------------------
          The Valid mappings for property names are:
          Valid Property name type combinations:
          property name: JCR Type, java Type
          name{byte}: long, byte
          name{short}: long, short
          name{int}: long, int
          name{float}: double, float
          name{char}: string, char
          name: string, string
          name: long, long
          name: double, double
          name: boolean, boolean

          If any {type}

          JCR type mapping not in this list is found, the whole config is rejected.

          When writing back:
          If the java type is one of byte,short,int, float,char then all matching properties of the form "name" and "name

          {type}" are deleted and a property of the form name{byte|short|int|float|char} is created of the appropriate type. (reusing existing if possible).
          If the java type is one of string, long, double, boolean then all matching properties of the form "name" and "name{type}

          " are deleted and a property of the form "name" is created with the appropriate type (if "name" of the appropriate types exists, its reused)

          Conversion from sling:OsgiConfig storage to OutputStream or Writer will:
          where the type is

          {byte}

          ,

          {short}

          ,

          {int}

          ,

          {float}

          ,

          {char}

          change the name to remove the

          {type} and write out in x"", s"", i"", f"", c"*" format.
          where the type is long, double, boolean keep the name and write out in l"",d"",b"*" format
          where the type is string and matches the value matches the pattern /^([TIiLlFfDdSsXxCcBb])\"(.)\"/ keep the name and write the value out as T"" ie T"T"text""
          where the type is string and does not match the pattern /^([TIiLlFfDdSsXxCcBb])\"(.*)\"/ keep the name and write the value out.

          Conversion from InputStream or Reader will read I"100" format and write name{type}

          format.
          where the value matches /^([IiFfSsXxCc])\"(.*)\"/ create a propery with name

          {int|float|short|byte|char}

          and write the encoded value.
          where the value matches /^([TLlDdBb])\"(.*)\"/ create a property of type string,long,double,boolean and set the raw value.
          where there is no match, create a property of the same name, type string and write the raw value.

          In all cases where special handling has to be performed for arrays.

          I"100" approach:
          --------------------------------------------
          Name, JCR Types, Java type

          name: long, long
          name: double, double
          name: boolean, boolean
          name: string, string where no match found for /^([TIiLlFfDdSsXxCcBb])\"(.*)\"/
          name: string, string where matching for /^(T)\"(.*)\"/
          name: string, integer where matching for /^([Ii])\"(.*)\"/
          name: string, float where matching for /^([Ff])\"(.*)\"/
          name: string, short where matching for /^([Ss])\"(.*)\"/
          name: string, byte where matching for /^([Xx])\"(.*)\"/
          name: string, character where matching for /^([Cc])\"(.*)\"/

          (optional if write back is not to change the type of a property from what a user set the value to)
          name: string, long where matching for /^([Ll])\"(.*)\"/
          name: string, double where matching for /^([Dd])\"(.*)\"/
          name: string, boolean where matching for /^([Bb])\"(.*)\"/

          Write back:
          If the name exists and is a string then the appropriate I"100" format is used.
          For types of string matching /^([TIiLlFfDdSsXxCcBb])\"(.*)\"/ the value is encoded as T"<value>"
          For types of long, double, boolean if the name does not exist an appropriate JCR property is created.
          For all other types a string property is created with the appropriate I"100" format.

          Conversion from sling:OsgiConfig to OutputStream or Writer will convert types long, double, boolean to the I"100" format, no name conversion required.
          Conversion from InputStream or Reader will
          where the value matches /^([LlDdBb])\"(.*)\"/ create a jcr property of the same name and type long, double, boolean with the extracted value, if the property exists and is a string, reuse it with the string format.
          for all other values create a string property of the same name and set the value.

          In all cases where special handling has to be performed for arrays.

          --------------------------------------------------------

          My view (non binding ) having just written the above is that the I"100" approach requires fewer rules and transitions to implement than the name

          {type}

          approach, which will require good unit test coverage to ensure a user can always edit a sling:OsgiConfig node and its properties, without ending up in a invalid state.

          I will create a Jira to track documentation of whatever approach is chosen since AFAICT neither are documented.

          Show
          Ian Boston added a comment - (apologies for saying "consensus" earlier). To summarise both approaches. name {type} approach: ------------------------------- The Valid mappings for property names are: Valid Property name type combinations: property name: JCR Type, java Type name{byte}: long, byte name{short}: long, short name{int}: long, int name{float}: double, float name{char}: string, char name: string, string name: long, long name: double, double name: boolean, boolean If any {type} JCR type mapping not in this list is found, the whole config is rejected. When writing back: If the java type is one of byte,short,int, float,char then all matching properties of the form "name" and "name {type}" are deleted and a property of the form name{byte|short|int|float|char} is created of the appropriate type. (reusing existing if possible). If the java type is one of string, long, double, boolean then all matching properties of the form "name" and "name{type} " are deleted and a property of the form "name" is created with the appropriate type (if "name" of the appropriate types exists, its reused) Conversion from sling:OsgiConfig storage to OutputStream or Writer will: where the type is {byte} , {short} , {int} , {float} , {char} change the name to remove the {type} and write out in x" ", s" ", i" ", f" ", c"*" format. where the type is long, double, boolean keep the name and write out in l" ",d" ",b"*" format where the type is string and matches the value matches the pattern /^( [TIiLlFfDdSsXxCcBb] )\"(. )\"/ keep the name and write the value out as T" " ie T"T"text"" where the type is string and does not match the pattern /^( [TIiLlFfDdSsXxCcBb] )\"(.*)\"/ keep the name and write the value out. Conversion from InputStream or Reader will read I"100" format and write name{type} format. where the value matches /^( [IiFfSsXxCc] )\"(.*)\"/ create a propery with name {int|float|short|byte|char} and write the encoded value. where the value matches /^( [TLlDdBb] )\"(.*)\"/ create a property of type string,long,double,boolean and set the raw value. where there is no match, create a property of the same name, type string and write the raw value. In all cases where special handling has to be performed for arrays. I"100" approach: -------------------------------------------- Name, JCR Types, Java type name: long, long name: double, double name: boolean, boolean name: string, string where no match found for /^( [TIiLlFfDdSsXxCcBb] )\"(.*)\"/ name: string, string where matching for /^(T)\"(.*)\"/ name: string, integer where matching for /^( [Ii] )\"(.*)\"/ name: string, float where matching for /^( [Ff] )\"(.*)\"/ name: string, short where matching for /^( [Ss] )\"(.*)\"/ name: string, byte where matching for /^( [Xx] )\"(.*)\"/ name: string, character where matching for /^( [Cc] )\"(.*)\"/ (optional if write back is not to change the type of a property from what a user set the value to) name: string, long where matching for /^( [Ll] )\"(.*)\"/ name: string, double where matching for /^( [Dd] )\"(.*)\"/ name: string, boolean where matching for /^( [Bb] )\"(.*)\"/ Write back: If the name exists and is a string then the appropriate I"100" format is used. For types of string matching /^( [TIiLlFfDdSsXxCcBb] )\"(.*)\"/ the value is encoded as T"<value>" For types of long, double, boolean if the name does not exist an appropriate JCR property is created. For all other types a string property is created with the appropriate I"100" format. Conversion from sling:OsgiConfig to OutputStream or Writer will convert types long, double, boolean to the I"100" format, no name conversion required. Conversion from InputStream or Reader will where the value matches /^( [LlDdBb] )\"(.*)\"/ create a jcr property of the same name and type long, double, boolean with the extracted value, if the property exists and is a string, reuse it with the string format. for all other values create a string property of the same name and set the value. In all cases where special handling has to be performed for arrays. -------------------------------------------------------- My view (non binding ) having just written the above is that the I"100" approach requires fewer rules and transitions to implement than the name {type} approach, which will require good unit test coverage to ensure a user can always edit a sling:OsgiConfig node and its properties, without ending up in a invalid state. I will create a Jira to track documentation of whatever approach is chosen since AFAICT neither are documented.
          Hide
          Ian Boston added a comment -

          Dont mark this issue as fixed until the documentation for the chosen approach (tracked by SLING-2756) has been done.

          Show
          Ian Boston added a comment - Dont mark this issue as fixed until the documentation for the chosen approach (tracked by SLING-2756 ) has been done.
          Hide
          Justin Edelson added a comment -

          @Ian - despite my reservations on the type"value" approach, I am very willing to accept that you have done more analysis on this than I have. As such, if you feel that the type"value" approach is less risky, than I think we should proceed with that approach.

          One note (perhaps my misunderstanding as to what you mean) - I don't think that conversion to/from OutputStream/InputStream/Reader/Writer is a requirement. After this change is made, a sling:OsgiConfig node should stay a sling:OsgiConfig node and a .config file should remain a .config file. No "conversion" is necessary - just parsing.

          Show
          Justin Edelson added a comment - @Ian - despite my reservations on the type"value" approach, I am very willing to accept that you have done more analysis on this than I have. As such, if you feel that the type"value" approach is less risky, than I think we should proceed with that approach. One note (perhaps my misunderstanding as to what you mean) - I don't think that conversion to/from OutputStream/InputStream/Reader/Writer is a requirement. After this change is made, a sling:OsgiConfig node should stay a sling:OsgiConfig node and a .config file should remain a .config file. No "conversion" is necessary - just parsing.
          Hide
          Ian Boston added a comment -

          @Justin I might have done more analysis, but I have a lot less experience of real use in this area so I am willing to do whatever consensus dictates. Eliminating conversion simplifies both approaches. I assume we need to support writeback as well as parsing ?

          Show
          Ian Boston added a comment - @Justin I might have done more analysis, but I have a lot less experience of real use in this area so I am willing to do whatever consensus dictates. Eliminating conversion simplifies both approaches. I assume we need to support writeback as well as parsing ?
          Hide
          Carsten Ziegeler added a comment -

          I think write back is a different issue - let's first define the format for the config nodes here and discuss write back in SLING-2752 - but I think write back should use the same format as has been used to read the configurations

          Show
          Carsten Ziegeler added a comment - I think write back is a different issue - let's first define the format for the config nodes here and discuss write back in SLING-2752 - but I think write back should use the same format as has been used to read the configurations
          Hide
          Ian Boston added a comment -

          Revisiting this, with trepidation since we didn't reach consensus.

          Since we should use the same format to writeback, and wanting to achieve consensus before doing anything, would it be wrong to support reading of both formats ?
          ie

          name

          {int}

          : "1000"
          and
          name: I"1000"

          It should be possible to accommodate both styles.

          Show
          Ian Boston added a comment - Revisiting this, with trepidation since we didn't reach consensus. Since we should use the same format to writeback, and wanting to achieve consensus before doing anything, would it be wrong to support reading of both formats ? ie name {int} : "1000" and name: I"1000" It should be possible to accommodate both styles.
          Hide
          Carsten Ziegeler added a comment -

          I didn't see a technical reason against the I"1000" notation, so let's go with this. Supporting two formats might create confusion which imho is not worth the potential/questionable benefit.
          As Sam Ruby mentioned around 2002: Apache is in theory a meritocracy but in practice a do-ocracy

          Show
          Carsten Ziegeler added a comment - I didn't see a technical reason against the I"1000" notation, so let's go with this. Supporting two formats might create confusion which imho is not worth the potential/questionable benefit. As Sam Ruby mentioned around 2002: Apache is in theory a meritocracy but in practice a do-ocracy
          Hide
          Alexander Klimetschek added a comment -

          Why is it always about technical reasons? This is a user interface and usability & readability should weigh higher than a minimal technical implementation. Everyone will have to look up those type chars ISX... all the time and whether you have to use quotes or not.

          And one technical reason is that the I"..." notation is not leveraging JCR types at all (and what a JCR browser/editor would give you as natural property editor for numbers, boolean etc.). -1 on adding types to a string format that are naturally supported by JCR types.

          Show
          Alexander Klimetschek added a comment - Why is it always about technical reasons? This is a user interface and usability & readability should weigh higher than a minimal technical implementation. Everyone will have to look up those type chars ISX... all the time and whether you have to use quotes or not. And one technical reason is that the I"..." notation is not leveraging JCR types at all (and what a JCR browser/editor would give you as natural property editor for numbers, boolean etc.). -1 on adding types to a string format that are naturally supported by JCR types.
          Hide
          Bertrand Delacretaz added a comment -

          I'm coming late to this, will try to avoid disturbing the discussion...it looks like having the type defined in the value is safer and more intuitive, but the name: I"1000" notation is very ugly and not self-explaining at all - where does it come from?

          As having to force those conversions is a somewhat unusual use case, it should be obvious that a conversion is taking place and what the target type is, so how about something like:

          service.ranking: ==java.lang.Integer==1000==

          where java.lang.Integer is an arbitrary class name that's supposed to have a constructor that takes a String argument?

          Show
          Bertrand Delacretaz added a comment - I'm coming late to this, will try to avoid disturbing the discussion...it looks like having the type defined in the value is safer and more intuitive, but the name: I"1000" notation is very ugly and not self-explaining at all - where does it come from? As having to force those conversions is a somewhat unusual use case, it should be obvious that a conversion is taking place and what the target type is, so how about something like: service.ranking: ==java.lang.Integer==1000== where java.lang.Integer is an arbitrary class name that's supposed to have a constructor that takes a String argument?
          Hide
          Ian Boston added a comment - - edited

          Where does it come from ?

          The I".." format is the same format as used by the Felix Configuration Admin that stores files under config/

          see org.apache.sling.commons.log.file.number below.

          I think that is why the first comment on this JIRA suggested using it. Using it would create a single format for config files under config/ and in sling:OsgiConfig. I have not been able to find definitive documentation from Apache Felix for the format.

          I am not certain where the org.apache.sling.commons.log.file.number

          {int} comes from, but it might be based on the JCR DocView XML format that allows properties to be named "{Integer}org.apache.sling.commons.log.file.number"=6. Unfortunate I suspect putting "{Integer}org.apache.sling.commons.log.file.number" into a Felix Config Admin file might be illegal as I suspect things starting with { are not valid, hence why org.apache.sling.commons.log.file.number{int}

          was suggested instead. I am not certain if the value should be quoted or not at that point. If its an existing format, perhaps someone can say where it comes from ?

          Regardless of which is chosen it will need to be documented and published in a location which can be found with ease.

          One of my big concerns was compatibility with existing formats and backward compatibility. There mist be literally 1000s of configuration files out there on disk for various reasons.

          To verify, configure something with an number in the Sling Console and look on disk.

          x43543-2:sling ieb$ more config/org/apache/sling/commons/log/LogManager/factory/writer/f5692976-ddb0-4ce2-b3d3-a254216981f6.config
          service.pid="org.apache.sling.commons.log.LogManager.factory.writer.f5692976-ddb0-4ce2-b3d3-a254216981f6"
          org.apache.sling.commons.log.file="logs/error.log"
          service.factoryPid="org.apache.sling.commons.log.LogManager.factory.writer"
          org.apache.sling.commons.log.file.number=I"6"
          org.apache.sling.commons.log.file.size="'.'yyyy-MM-dd"
          x43543-2:sling ieb$

          Show
          Ian Boston added a comment - - edited Where does it come from ? The I".." format is the same format as used by the Felix Configuration Admin that stores files under config/ see org.apache.sling.commons.log.file.number below. I think that is why the first comment on this JIRA suggested using it. Using it would create a single format for config files under config/ and in sling:OsgiConfig. I have not been able to find definitive documentation from Apache Felix for the format. I am not certain where the org.apache.sling.commons.log.file.number {int} comes from, but it might be based on the JCR DocView XML format that allows properties to be named "{Integer}org.apache.sling.commons.log.file.number"=6. Unfortunate I suspect putting "{Integer}org.apache.sling.commons.log.file.number" into a Felix Config Admin file might be illegal as I suspect things starting with { are not valid, hence why org.apache.sling.commons.log.file.number{int} was suggested instead. I am not certain if the value should be quoted or not at that point. If its an existing format, perhaps someone can say where it comes from ? Regardless of which is chosen it will need to be documented and published in a location which can be found with ease. One of my big concerns was compatibility with existing formats and backward compatibility. There mist be literally 1000s of configuration files out there on disk for various reasons. To verify, configure something with an number in the Sling Console and look on disk. x43543-2:sling ieb$ more config/org/apache/sling/commons/log/LogManager/factory/writer/f5692976-ddb0-4ce2-b3d3-a254216981f6.config service.pid="org.apache.sling.commons.log.LogManager.factory.writer.f5692976-ddb0-4ce2-b3d3-a254216981f6" org.apache.sling.commons.log.file="logs/error.log" service.factoryPid="org.apache.sling.commons.log.LogManager.factory.writer" org.apache.sling.commons.log.file.number=I"6" org.apache.sling.commons.log.file.size="'.'yyyy-MM-dd" x43543-2:sling ieb$
          Hide
          Alexander Klimetschek added a comment -

          We are discussion a JCR optimized, user front-end format here, so I think there is no special reason why one should use the internal Felix Configuration Admin format as basis (which for sure is optimized for a) being a text file format and b) short value strings). Nobody on the Sling/JCR stack is supposed to look at those internal felix config files and convert them back and forth against the repository format... so these are two completely separate topics. The initial sling:OsgiConfig node type was designed completely separate from whatever the OSGi implementation would do and for a good reason.

          The property

          {int}

          style is not coming anywhere specific I think. There are three factors that make it up:

          • put type into property name so the value is not forced to be a string
          • this allows to use jcr LONG for integer, DOUBLE for float etc. and give better input editors in e.g. JCR browsers or when writing those values programmatically using the JCR API
          • then use a readable & rememberable type name such as "byte", "int", "float" instead of X, I, F
          • as uncommon delimiter use curly braces {...}

            (we might want to add escaping in the form of "{" and "
            " to be 100% safe)

          before someone mentions that one could break the limits of those smaller types: the config node parser has to take care of those type checks anyway - in case of the string formats it has to do that as well, someone could exceed the limits of this data type there and additionally write garbage such as I"12this_is_not_a_number".

          Show
          Alexander Klimetschek added a comment - We are discussion a JCR optimized, user front-end format here, so I think there is no special reason why one should use the internal Felix Configuration Admin format as basis (which for sure is optimized for a) being a text file format and b) short value strings). Nobody on the Sling/JCR stack is supposed to look at those internal felix config files and convert them back and forth against the repository format... so these are two completely separate topics. The initial sling:OsgiConfig node type was designed completely separate from whatever the OSGi implementation would do and for a good reason. The property {int} style is not coming anywhere specific I think. There are three factors that make it up: put type into property name so the value is not forced to be a string this allows to use jcr LONG for integer, DOUBLE for float etc. and give better input editors in e.g. JCR browsers or when writing those values programmatically using the JCR API then use a readable & rememberable type name such as "byte", "int", "float" instead of X, I, F as uncommon delimiter use curly braces {...} (we might want to add escaping in the form of "{" and " " to be 100% safe) before someone mentions that one could break the limits of those smaller types: the config node parser has to take care of those type checks anyway - in case of the string formats it has to do that as well, someone could exceed the limits of this data type there and additionally write garbage such as I"12this_is_not_a_number".
          Hide
          Carsten Ziegeler added a comment -

          Let's stop this endless discussion and implement it in one way or the other; we can't reach consensus, some people prefer blue others prefer red - who is right? The one drawing the image picks the color in this case.

          Show
          Carsten Ziegeler added a comment - Let's stop this endless discussion and implement it in one way or the other; we can't reach consensus, some people prefer blue others prefer red - who is right? The one drawing the image picks the color in this case.
          Hide
          Ian Boston added a comment -

          +1.
          I'll try and support both formats as they are not mutually exclusive. Its more work, but less than trying to reach consensus.

          Show
          Ian Boston added a comment - +1. I'll try and support both formats as they are not mutually exclusive. Its more work, but less than trying to reach consensus.
          Hide
          Carsten Ziegeler added a comment -

          Please, don't do two different formats - that's really not worth it and creates too much confusion. As we have so vocal people here for name

          {int}

          : "1000" than this be it - it's against my believe, but in the end I'll never handcraft configurations in the repository by hand. And as soon as the first problem with that format arises I know at least who to call.

          Show
          Carsten Ziegeler added a comment - Please, don't do two different formats - that's really not worth it and creates too much confusion. As we have so vocal people here for name {int} : "1000" than this be it - it's against my believe, but in the end I'll never handcraft configurations in the repository by hand. And as soon as the first problem with that format arises I know at least who to call.
          Hide
          Ilyas Türkben added a comment -

          In Sling, the class org.apache.sling.engine.impl.filter.ServletFilterManager:212(org.apache.sling.engine-2.3.10.jar) expects an Integer in order to set the Filter ranking which makes impossible defining filter rankings via sling:OsgiConfig nodes.

          Show
          Ilyas Türkben added a comment - In Sling, the class org.apache.sling.engine.impl.filter.ServletFilterManager:212(org.apache.sling.engine-2.3.10.jar) expects an Integer in order to set the Filter ranking which makes impossible defining filter rankings via sling:OsgiConfig nodes.
          Hide
          Carsten Ziegeler added a comment -

          Services ranking must be an integer, this is demanded by the OSGi spec.
          Ilyas Türkben Instead of using a config node, a config file can be used which works out of the box

          Show
          Carsten Ziegeler added a comment - Services ranking must be an integer, this is demanded by the OSGi spec. Ilyas Türkben Instead of using a config node, a config file can be used which works out of the box
          Hide
          Ian Boston added a comment -

          UnAssigning myself as I was unable to reach consensus on this issue in 2013, perhaps someone else would like to have a go at reaching consensus.

          Show
          Ian Boston added a comment - UnAssigning myself as I was unable to reach consensus on this issue in 2013, perhaps someone else would like to have a go at reaching consensus.

            People

            • Assignee:
              Unassigned
              Reporter:
              Alexander Klimetschek
            • Votes:
              0 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:

                Development