Correctly translate Cellular Device properties to ONC

Correctly translate Cellular Device properties to ONC

This fixes how we translate the Device properties to ONC instead of
just copying the Shill properties.

BUG=279351
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285642

[stevenjb, 2014-07-18 20:52:58]

When I added the Device properties to the Shill -> ONC translation, I really
didn't know what I was doing. I still don't really, so please review this
carefully, but I think it is at least more correct.

[armansito, 2014-07-18 21:21:52]

https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:289:
TranslateAndAddNestedObject(::onc::cellular::kSIMLockStatus, *dictionary);
I wonder if we should just place all the nested fields of the shill
SIMLockStatus property at the top level device properties dictionary to avoid
yet another nested entry. I don't have a strong opinion on this (since the way
you have it is at least consistent with what's returned from shill) but I have a
feeling that all the nested dictionaries end up complicating the data that we
have to deal with while using networkingPrivate.

[stevenjb, 2014-07-18 21:31:50]

Note, I found some related translation cleanup that I will do in this CL also,
will have an updated patch soon.

https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:289:
TranslateAndAddNestedObject(::onc::cellular::kSIMLockStatus, *dictionary);
On 2014/07/18 21:21:52, armansito wrote:
> I wonder if we should just place all the nested fields of the shill
> SIMLockStatus property at the top level device properties dictionary to avoid
> yet another nested entry. I don't have a strong opinion on this (since the way
> you have it is at least consistent with what's returned from shill) but I have
a
> feeling that all the nested dictionaries end up complicating the data that we
> have to deal with while using networkingPrivate.

Fortunately in JS, nested dictionaries are easy to deal with, and in some ways
it is actually more convenient to have a nested dictionary of related
properties.

Now, it would be nice if we would automatically handle translation of these
dictionaries, and I considered looking into that, but decided that I would
probably be more work than I am willing to do right now.

[armansito, 2014-07-18 21:37:23]

On 2014/07/18 21:31:50, stevenjb wrote:
> Note, I found some related translation cleanup that I will do in this CL also,
> will have an updated patch soon.
> 
>
https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
> File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):
> 
>
https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
> chromeos/network/onc/onc_translator_shill_to_onc.cc:289:
> TranslateAndAddNestedObject(::onc::cellular::kSIMLockStatus, *dictionary);
> On 2014/07/18 21:21:52, armansito wrote:
> > I wonder if we should just place all the nested fields of the shill
> > SIMLockStatus property at the top level device properties dictionary to
avoid
> > yet another nested entry. I don't have a strong opinion on this (since the
way
> > you have it is at least consistent with what's returned from shill) but I
have
> a
> > feeling that all the nested dictionaries end up complicating the data that
we
> > have to deal with while using networkingPrivate.
> 
> Fortunately in JS, nested dictionaries are easy to deal with, and in some ways
> it is actually more convenient to have a nested dictionary of related
> properties.
> 
> Now, it would be nice if we would automatically handle translation of these
> dictionaries, and I considered looking into that, but decided that I would
> probably be more work than I am willing to do right now.

That's true. It would be nice if all of these mappings could be provided in a
simple JSON like file and the translation code autogenerated (or it least be
made more generic). Though, I guess this is fine for now.

lgtm, though have pneubeck@ take a look at it as well.

[stevenjb, 2014-07-18 21:43:40]

OK, added fixes for APN and FoundNetworks. I am definitely planning to wait on
Philipp for this CL, it's not super urgent :)

[pneubeck (no reviews), 2014-07-21 08:12:50]

On 2014/07/18 21:37:23, armansito wrote:
> On 2014/07/18 21:31:50, stevenjb wrote:
> > Note, I found some related translation cleanup that I will do in this CL
also,
> > will have an updated patch soon.
> > 
> >
>
https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
> > File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):
> > 
> >
>
https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
> > chromeos/network/onc/onc_translator_shill_to_onc.cc:289:
> > TranslateAndAddNestedObject(::onc::cellular::kSIMLockStatus, *dictionary);
> > On 2014/07/18 21:21:52, armansito wrote:
> > > I wonder if we should just place all the nested fields of the shill
> > > SIMLockStatus property at the top level device properties dictionary to
> avoid
> > > yet another nested entry. I don't have a strong opinion on this (since the
> way
> > > you have it is at least consistent with what's returned from shill) but I
> have
> > a
> > > feeling that all the nested dictionaries end up complicating the data that
> we
> > > have to deal with while using networkingPrivate.
> > 
> > Fortunately in JS, nested dictionaries are easy to deal with, and in some
ways
> > it is actually more convenient to have a nested dictionary of related
> > properties.
> > 
> > Now, it would be nice if we would automatically handle translation of these
> > dictionaries, and I considered looking into that, but decided that I would
> > probably be more work than I am willing to do right now.
> 
> That's true. It would be nice if all of these mappings could be provided in a
> simple JSON like file and the translation code autogenerated (or it least be
> made more generic). Though, I guess this is fine for now.

Using JSON schema for the signature/type definition would surely be nice. This
exists already for extension APIs.

But how do you imagine will a JSON like file help for the translation?
The important design decision for the translation was that it must be easy to
implement special cases.
That's why we ended up with the ability to overwrite the translation of any
dictionary/field.

The tables of the simple translations (just the mapping shill name <-> onc name)
is already the most compact representation. JSON wouldn't improve that.

Another design decision was to separate ONC from the translation. I think it
would be a step backwards to for example integrate translation annotations in
the signature definition (in case you had that in mind for the json thing).

> 
> lgtm, though have pneubeck@ take a look at it as well.

[pneubeck (no reviews), 2014-07-21 08:20:18]

On 2014/07/18 21:31:50, stevenjb wrote:
> Note, I found some related translation cleanup that I will do in this CL also,
> will have an updated patch soon.
> 
>
https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
> File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):
> 
>
https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
> chromeos/network/onc/onc_translator_shill_to_onc.cc:289:
> TranslateAndAddNestedObject(::onc::cellular::kSIMLockStatus, *dictionary);
> On 2014/07/18 21:21:52, armansito wrote:
> > I wonder if we should just place all the nested fields of the shill
> > SIMLockStatus property at the top level device properties dictionary to
avoid
> > yet another nested entry. I don't have a strong opinion on this (since the
way
> > you have it is at least consistent with what's returned from shill) but I
have
> a
> > feeling that all the nested dictionaries end up complicating the data that
we
> > have to deal with while using networkingPrivate.
> 
> Fortunately in JS, nested dictionaries are easy to deal with, and in some ways
> it is actually more convenient to have a nested dictionary of related
> properties.
> 
> Now, it would be nice if we would automatically handle translation of these
> dictionaries, and I considered looking into that, but decided that I would
> probably be more work than I am willing to do right now.

Could you briefly explain how any of the translation could/should be automated?
I guess you mean (e.g. for Shill -> ONC translation), that nested ONC objects
could automatically be generated and pick up props from the shill dictionary?
One problem is that the source dictionaries have to be defined

Second problem is that most service properties are in a single flat dictionary
and it's unclear which nested ONC object (out of many) is the right one to
create.
E.g. imagine NetworkConfiguration: Which nested object to create should depend
on the 'Type' property and can lead to one of Ethernet/Wifi/Cellular/...

We could probably just create all nested ONC objects and delete empty ones
afterwards and additionally apply the ONC normalization afterwards (which
deletes 'irrelevant' fields, e.g. if Type=wifi it will delete a nested Ethernet
object).
I just fear that such behavior is even harder to grasp/debug :-/

[pneubeck (no reviews), 2014-07-21 08:21:53]

I like this code much better than the old one. It's more consistent with the
other translations.

Please verify it with unit tests (onc_translator_unittest.cc): at least to
ensure that nested objects are correctly picked up.

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_signature.cc:264: { ::onc::cellular::kAPNList,
&kCellularApnListSignature},
this exists now in both CellularDevice and Cellular (and by 'inheritance' as
well in CellularWithState).
is one redundant?

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:265: // CellularDevice
signiture.
typo: signiture -> signature

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:280: const
base::DictionaryValue* dictionary = NULL;
nit: dictionary -> shill_sim_lock

[pneubeck (no reviews), 2014-07-21 08:25:55]

Oh. But now I remember why we had all these properties in the Cellular
signature:

Before this change, the signature described the object that is returned from the
translation.
Now, it doesn't anymore :-/
The cellular object contains fields that are not in the CellularSignature.

If we ever want to apply e.g. validation to such objects then we can't rely
anymore on the signature.

[pneubeck (no reviews), 2014-07-21 08:31:05]

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:272:
kCellularDeviceSignature);
how about using kCellularWithStateSignature here again
but additionally passing it the translation table |cellular_device_fields| to
use (you will have to adapt the constructor to set a custom translation table) ?

[stevenjb, 2014-07-22 18:29:18]

On 2014/07/21 08:25:55, pneubeck wrote:
> Oh. But now I remember why we had all these properties in the Cellular
> signature:
> 
> Before this change, the signature described the object that is returned from
the
> translation.
> Now, it doesn't anymore :-/
> The cellular object contains fields that are not in the CellularSignature.
> 
> If we ever want to apply e.g. validation to such objects then we can't rely
> anymore on the signature.

I don't really understand the difference between "Cellular" and
"CellularWithState" to be honest. I haven't looked at the import code at all
where I assume the signature is more important.

I think I may kind of understand what you're suggesting, but maybe you can
clarify:
* Put the Device properties back in the CellularWithState signature
* Pass the device field translation table as an additional argument to the
nested translator instead of looking it up by signature

I will take a stab at that and see if I can make it work reasonably quickly.

[stevenjb, 2014-07-22 18:29:33]

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_signature.cc:264: { ::onc::cellular::kAPNList,
&kCellularApnListSignature},
On 2014/07/21 08:21:53, pneubeck wrote:
> this exists now in both CellularDevice and Cellular (and by 'inheritance' as
> well in CellularWithState).
> is one redundant?

I think it probably doesn't belong in Cellular, will remove.

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:265: // CellularDevice
signiture.
On 2014/07/21 08:21:53, pneubeck wrote:
> typo: signiture -> signature

Done.

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:272:
kCellularDeviceSignature);
On 2014/07/21 08:31:04, pneubeck wrote:
> how about using kCellularWithStateSignature here again
> but additionally passing it the translation table |cellular_device_fields| to
> use (you will have to adapt the constructor to set a custom translation table)
?

I'm not sure I follow.

https://codereview.chromium.org/402953004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:280: const
base::DictionaryValue* dictionary = NULL;
On 2014/07/21 08:21:53, pneubeck wrote:
> nit: dictionary -> shill_sim_lock

Done.

[pneubeck (no reviews), 2014-07-22 18:43:00]

On 2014/07/22 18:29:18, stevenjb wrote:
> On 2014/07/21 08:25:55, pneubeck wrote:
> > Oh. But now I remember why we had all these properties in the Cellular
> > signature:
> > 
> > Before this change, the signature described the object that is returned from
> the
> > translation.
> > Now, it doesn't anymore :-/
> > The cellular object contains fields that are not in the CellularSignature.
> > 
> > If we ever want to apply e.g. validation to such objects then we can't rely
> > anymore on the signature.
> 
> I don't really understand the difference between "Cellular" and
> "CellularWithState" to be honest. I haven't looked at the import code at all
> where I assume the signature is more important.
> 
> I think I may kind of understand what you're suggesting, but maybe you can
> clarify:
> * Put the Device properties back in the CellularWithState signature
> * Pass the device field translation table as an additional argument to the
> nested translator instead of looking it up by signature
> 
> I will take a stab at that and see if I can make it work reasonably quickly.

Yes, this matches with what I had in mind.

The difference between Cellular and CellularWithState is, like any X and
XWithState, that CellularWithState contains additional state or read-only
properties that Shill provides.
We separated such properties into a separate signature, so that the original
signature (Cellular or X) can still be used for validating input configurations
(form policy or the API).
Also should signatures that are used for policy validation (e.g. Wifi) conform
to the onc_spec, which doesn't contain any such state properties (yet).

[armansito, 2014-07-22 21:39:36]

On 2014/07/21 08:12:50, pneubeck wrote:
> On 2014/07/18 21:37:23, armansito wrote:
> > On 2014/07/18 21:31:50, stevenjb wrote:
> > > Note, I found some related translation cleanup that I will do in this CL
> also,
> > > will have an updated patch soon.
> > > 
> > >
> >
>
https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
> > > File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/402953004/diff/20001/chromeos/network/onc/onc...
> > > chromeos/network/onc/onc_translator_shill_to_onc.cc:289:
> > > TranslateAndAddNestedObject(::onc::cellular::kSIMLockStatus, *dictionary);
> > > On 2014/07/18 21:21:52, armansito wrote:
> > > > I wonder if we should just place all the nested fields of the shill
> > > > SIMLockStatus property at the top level device properties dictionary to
> > avoid
> > > > yet another nested entry. I don't have a strong opinion on this (since
the
> > way
> > > > you have it is at least consistent with what's returned from shill) but
I
> > have
> > > a
> > > > feeling that all the nested dictionaries end up complicating the data
that
> > we
> > > > have to deal with while using networkingPrivate.
> > > 
> > > Fortunately in JS, nested dictionaries are easy to deal with, and in some
> ways
> > > it is actually more convenient to have a nested dictionary of related
> > > properties.
> > > 
> > > Now, it would be nice if we would automatically handle translation of
these
> > > dictionaries, and I considered looking into that, but decided that I would
> > > probably be more work than I am willing to do right now.
> > 
> > That's true. It would be nice if all of these mappings could be provided in
a
> > simple JSON like file and the translation code autogenerated (or it least be
> > made more generic). Though, I guess this is fine for now.
> 
> Using JSON schema for the signature/type definition would surely be nice. This
> exists already for extension APIs.
> 
> But how do you imagine will a JSON like file help for the translation?
> The important design decision for the translation was that it must be easy to
> implement special cases.
> That's why we ended up with the ability to overwrite the translation of any
> dictionary/field.
> 
> The tables of the simple translations (just the mapping shill name <-> onc
name)
> is already the most compact representation. JSON wouldn't improve that.
> 
> Another design decision was to separate ONC from the translation. I think it
> would be a step backwards to for example integrate translation annotations in
> the signature definition (in case you had that in mind for the json thing).
> 

I'm actually fine with the way things are right now. The only reason I bring
this up is because I feel that it's not easily obvious to an outsider how one
can add new translation entries without having to touch several distinct places
in the code (new constant definitions, different signature and table entries),
and in the case of nested dictionaries, adding special cases to the translation
logic itself, as opposed to perhaps just editing a mapping file of sorts. I'm
obviously not an expert here and I'm actually curious how other platforms
(non-ChromeOS) are handling this and whether or not they would benefit from such
a thing as well. I'm mostly thinking out loud here.

[stevenjb, 2014-07-24 22:24:33]

OK, I *think* this is better. Also added more tests, although it turns out we
did already have tests and one broke because the APNList was in the wrong place,
so yay tests (even though the test itself happened to be wrong). PTAL.

[pneubeck (no reviews), 2014-07-25 08:50:03]

lgtm

https://codereview.chromium.org/402953004/diff/60001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/402953004/diff/60001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:144: if
(field_translation_table_ == kCellularDeviceTable)
oh, I didn't foresee that one :-/
To avoid this, I think we would need a separate class with other translation
functions.
For this one special case however, I think it's fine.

[stevenjb, 2014-07-25 17:00:54]

https://codereview.chromium.org/402953004/diff/60001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/402953004/diff/60001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:144: if
(field_translation_table_ == kCellularDeviceTable)
On 2014/07/25 08:50:03, pneubeck wrote:
> oh, I didn't foresee that one :-/
> To avoid this, I think we would need a separate class with other translation
> functions.
> For this one special case however, I think it's fine.

Yeah, I'll be honest, I find these translation functions awkward and difficult
to maintain. It would be nice to eliminate the need for the majority of the
special handling and have a single special case handler based on property name.
Or something. In any case, this seemed like a reasonable incremental step
forward, glad you agree :)

[stevenjb, 2014-07-25 17:01:03]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-07-25 17:02:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/402953004/60001

[commit-bot: I haz the power, 2014-07-25 19:04:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)

[commit-bot: I haz the power, 2014-07-25 19:14:34]

Change committed as 285642