[FEATURE] acc module acc_extra database types other than string

[ar45]

• Component: acc module
• Type: acc_extra

I am using acc_extra vars to store additional information for CDR records such as pdd, billing etc. Currently, all those vars are coerced and inserted to the database as string values.

It would be beneficial if we can configure the type of those vars so we can insert integer or decimal values as well.

Currently, I have to do extra work using a trigger or external job to transform the data.

[greenbea]

I have the same issue with acc_extra saving unset variables as empty strings forcing me to initialize all my acc_extra variables that can possibly be empty to match the data type I use in postgres

[liviuchircu]

Would attaching a type to each custom field satisfy all of your requirements, @ar45 and @greenbea? There would be two possible types to choose from: int and str... which should cover all possible, respective subtypes (e.g. TINYINT, SMALLINT, FLOAT, DOUBLE... for integers) and (TEXT, BLOB, CHAR, VARCHAR... for strings). Here's how this would look:

modparam("acc", "leg_fields", "
    db:
        trunk_id:int;
        trunk_ip:str;
        dial_in:str;
        my_gw_id->gw_id:int;
        my_carrier_id->carrier_id:str;
        gw_dial;
        tech_prefix;
        call_direction:int
        ")

EDIT: perhaps we should also consider a float type, so the casting of OpenSIPS script values resembling, e.g. "10.137", will be done without losing the fractional part of the number.

EDIT: the default value would also change, from "" (empty-string), to DEFAULT, thus becoming compatible with any column type and its default value chosen by the developer.

[ar45]

@liviuchircu Yes. That definitely satisfies. It is also important to be able to tell whether the value should be NULL when unset or empty string or maybe the keyword DEFAULT which is valid for INSERT INTO tbl (col, ...) VALUES(DEFAULT, ...)

[liviuchircu]

Yes, the default would be to set the value to DEFAULT, rather than "". Let me update the comment-specification...

[kingsleytart]

I would like this too. I think setting to DEFAULT is the tidiest solution, and probably the easiest to implement.

[liviuchircu]

If you think about it... we could backport the "set to DEFAULT" part immediately, as a fix (it seems to only do good, with no side-effects, right?). However, we need to make sure we don't break the esoteric SQL backends, such as db_flatstore and db_text while doing so.

[kingsleytart]

I would love to say yes, but some people might not have default values defined in some of their DB fields, at which point such a change would make their queries fail, eg:

MariaDB [opensips]> INSERT INTO acc (time) VALUES (DEFAULT);
ERROR 1364 (HY000): Field 'time' doesn't have a default value
MariaDB [opensips]>

Is it feasible to introduce a module parameter which, if set, enables this behaviour? eg

modparam("acc", "acc_extra_field_defaults", 1);

or some other name if you prefer.

For my own purposes I would prefer it to just use DEFAULT but I think it could break existing setups for other people.

Unless, somehow, you can make this very obvious in the release notes for a new version, with old versions requiring the parameter, or perhaps having a different default value for that parameter?

[greenbea]

I also think besides being able to choose the data type having the option of NULL and DEFAULT would be beneficial

[ar45]

The reason I thought of using DEFAULT was also because of the ability to backport and the simplicity and easy to implement. But we might need a little more than that.

Long term and covering other usecases would be to have flags on a avp or pvar to indicate whether the value is to be quoted and/or escaped and handle NULL automatically. For example $avp(name:<q>) for quoting and SQLEscaping the value all the time. A null value will be an empty string. $avp(name:<Q>) for quoting and SQLEscaping the value if it is non null. $avp(name:<i>) for quoting a SQL identifier. $avp(name:<k>) for an unquoted string, which can be used as a SQL KEYWORD like DEFAULT or CURRENT_TIMESTAMP, etc.

Or maybe have the flags as printf style formatting $avp(name:<%s>) and $avp(name:<%Q>), $avp(name:<%.2f>)

[ar45]

The above makes me think about how to handle avp array values. Maybe each index should be able to have it's own flags?

[ar45]

Also, I still need a little more complex scenarios for datatypes like postgres ARRAY text[]. An array can be written in postgres using {a,b,c} which would need an implicit cast, explicit cast {a,b,c}::text[], or using the array constructor ARRAY[ 'a', 'b', 'c' ] where each value needs to be individually quoted.

In the past, I've used multi value avps for array values, as in $avp(my-array-construct) = "{'" + $avp(name)[*] + "'}". I had to make sure before to escape each value using string transformations.

I'm thinking how we'd be able to simplify that using flags.

There are 2 ways to consider the flags, one is for the avp while storing the value, indicating what type of value it holds, the other is for extracting the value upon use. For the latter we can use transformations, these transformations can be implemented by the underlying database being used. so avp_db_query will use the appropriate transform function exported by that database URL in use.

Edit: Really, all we need is a way to tell the acc module how to transform or encode our value. so how about the following:

modparam("acc", "leg_fields", "
    db:
        trunk_id:{db.int};
        trunk_ip:{db.inet};            # for postgres db only which would implement inet datatype to validate it is a valid ip address
        dial_in:{db.escape_quoted};
        my_gw_id->gw_id:{db.keyword};
        rate->call_rate:{s.decimal,1,3};      # decimal precision
        id:{s.fill.left, 0, 6}                           # pad value
        use_default_keyword_if_null:{db.default,DEFAULT}{db.quote_if_not_keyword}
        use_provided_value_as_default:{db.default,\"\"}{db.escape_quoted}
        ")

The key to this, 1) implementing database transformations on the db modules. 2) use transformations for acc variables when extracting the value before storing/logging.

[ar45]

Related to this #1530