Merge lp://qastaging/~justin-fathomdb/nova/output-xml-in-order into lp://qastaging/~hudson-openstack/nova/trunk

The diff created by launchpad seems to be missing a lot. For example, you added a api/openstack/validation.py in rev 849. Nowhere in the diff is that file referenced. I also don't see the changes you made in wsgi.py. This probably isn't your fault, I just wanted to point this out to other reviewers. I used the following command to generate a diff:

bzr diff lp:~justin-fathomdb/nova/output-xml-in-order --old=lp:nova/trunk -r 949

So it looks like this branch does more than "Allow XML elements to be listed, and then output in that order". Can you expand on that?

Does this branch also depend on lp:~justin-fathomdb/nova/add-xsd? Otherwise, why are you adding all of the xsd files in this branch?

I also noticed "# NOTE(justinsb): Commented out for now because it finds too many issues" in fake_flags.py. Is this the right approach to take? Why don't we address the "issues" before we merge this in?

Can you explain what the "validate_xml_for_json" flag means?

Hi Brian,

You're right - there are multiple pre-reqs. LP doesn't let me do that (?), so I guess I'm going to repoint the prereq as the prereqs merge. I believe the only additional pre-req is my check-xsd branch (which itself depends on add-xsd). Makes the diff not very useful at the moment :-(

The comment about all the commented out flags and validate_xml_for_json are really about check-xsd. But anyway:

validate_xml_for_json will generate (and discard) XML even when JSON is requested. It will therefore check the JSON. It's really for dev/testing purposes (the system can check the XML for you against the spec even if you don't like XML!)

However, this does find a huge number of issues. I tried attacking some of them last night, but there's a lot of work there. Many of them are real bugs, some of them are in the JSON also. For example, test_resized_server_has_correct_status gives this error:

2011-04-07 07:57:23,412 WARNING nova.api.openstack [-] XML did not validate: Element '{http://docs.rackspacecloud.com/servers/api/v1.0}server', attribute 'status': [facet 'enumeration'] The value 'RESIZE-CONFIRM' is not an element of the set {'ACTIVE', 'SUSPENDED', 'DELETED', 'QUEUE_RESIZE', 'PREP_RESIZE', 'RESIZE', 'VERIFY_RESIZE', 'RESCUE', 'ERROR', 'BUILD', 'RESTORING', 'PASSWORD', 'REBUILD', 'DELETE_IP', 'SHARE_IP_NO_CONFIG', 'SHARE_IP', 'REBOOT', 'HARD_REBOOT', 'UNKNOWN', 'QUEUE_MOVE', 'PREP_MOVE', 'MOVE', 'VERIFY_MOVE', 'PENDING'}., line 1
XML=<server flavorId="1" hostId="" id="1" imageId="10" name="server1" status="RESIZE-CONFIRM" xmlns="http://docs.rackspacecloud.com/servers/api/v1.0">
    <metadata>
        <meta key="seq">
            1
        </meta>
    </metadata>
    <addresses>
        <public/>
        <private/>
    </addresses>
</server>

So I think RESIZE-CONFIRM is used where it should be VERIFY_RESIZE, but I'm not sure here.

However, the real problem is that right now the XML schema for the OSAPI 1.1 won't even load. I've opened a bug for it (Bug #752929), but I'm really discouraged by that. I did change it in another branch so that it just skips the 1.1 schema, but really it means we're skipping half the validation we should be doing.

Sorry Brian - I'm still pre-coffee... The key point is that there are way too many issues for me to solve pre-merge, so by putting them in with a flag that anyone can easily enable, then everyone can easily put the flags back in and see the issues it uncovers. I think that the commented out flags is the right thing to do for now.

No worries, Justin. I still like the approach that we would add the schemas piece by piece so we don't introduce a whole lot of "failing" code. I don't think this should go in for cactus, as we would essentially be shipping a broken feature.

As for the validate_xml_for_json, why don't we use json schemas? I don't think we can assume the json is correct if the xml validates correctly. It's really a caveat of how we do serialization right now, and may not be something to trust in the future.

I am also a bit weary of your use of prerequisites. I feel like you could be grouping your like-minded branches into one, rather than creating complex webs of prereqs. When you have multiple levels of branches that depend on each other, then you merge in trunk multiple times, it becomes very difficult to see what the actual diff should be. You would have a better chance of getting your code in if you would group it more logically so the reviewers could easily see what you are proposing.

On pre-reqs, I know that I push DVCS a bit too hard, and don't use it
correctly in other places! I haven't figured out how to split out
commits easily with bazaar yet - hopefully someone will give a master
class at the Design Summit.

Though we're not schema compliant, I think shipping the schemas is
just honest. The schemas are readily available on the rackspace
website (1.0) and in our own documentation (1.1). Sadly, not shipping
the schemas won't make our output correct. I think adding them
piecemeal is just a bit of a waste of time when they're already
available elsewhere.

I am all for adding JSON schemas. In the meantime, if our JSON was
strictly derived from our XML (as it probably should be), then
validating our XML would validate our JSON, and everything would be
much easier. Otherwise, checking that we have valid XML helps
developers check their output. This is not enabled by default in
production, nor even in the unit tests at the moment.

This doesn't cause any unit tests to fail unless you tweak the flags
to go into 'strict mode', BTW.

> I am all for adding JSON schemas. In the meantime, if our JSON was
> strictly derived from our XML (as it probably should be), then
> validating our XML would validate our JSON, and everything would be
> much easier.

I completely disagree that JSON should be derived from XML output. While it might seem to make validation easier right now, tightly coupling output like that is a recipe for disaster down the line.

> On pre-reqs, I know that I push DVCS a bit too hard, and don't use it
> correctly in other places! I haven't figured out how to split out
> commits easily with bazaar yet - hopefully someone will give a master
> class at the Design Summit.

I could definitely use this, too.

> Though we're not schema compliant, I think shipping the schemas is
> just honest. The schemas are readily available on the rackspace
> website (1.0) and in our own documentation (1.1). Sadly, not shipping
> the schemas won't make our output correct. I think adding them
> piecemeal is just a bit of a waste of time when they're already
> available elsewhere.

What does adding the schemas to the project get us if they are already all available and easily accessible? The major reason to add the schemas is to validate input/output. Right now, adding the schemas isn't adding much value to the product.

> I am all for adding JSON schemas. In the meantime, if our JSON was
> strictly derived from our XML (as it probably should be), then
> validating our XML would validate our JSON, and everything would be
> much easier. Otherwise, checking that we have valid XML helps
> developers check their output.

JSON and XML are two *different* outputs. We will align them, but one shouldn't depend on the other. With your logic, we could use XML objects to represent things internally, then serialize it to JSON in a response.

> This is not enabled by default in
> production, nor even in the unit tests at the moment.
>
> This doesn't cause any unit tests to fail unless you tweak the flags
> to go into 'strict mode', BTW.

This brings me back to my point above, what real value are we getting if we don't strictly enforce the schemas?

It isn't a validation argument Brian (L!). My logic is that one
should derive from the other, so that we don't have to double our
workload and can focus on just getting it right. There's no real
difference between XML and JSON really. XML has attributes (probably
an unnecessary complexity), JSON doesn't have namespaces (yet), the
syntax is a little different with XML naming elements on close while
JSON doesn't, and JSON has anonymous arrays while XML doesn't.

So, as developers, we should be looking to make our life easier - why
write something twice when you can write it once?

XML has more information, so the easy way is to output the one with
more information and then it's a simple projection to transform it
into the one that has less information.

The next best alternative IMHO is to output the one with less
information and then decorate it with the additional information.
That's a lot harder.

I think treating XML and JSON separately comes last in my ranking list.

> With your logic, we could use XML objects to represent things internally, then serialize it to JSON in a response.

Do you mean using strongly typed domain objects, so that we wipe out half our bugs in a single stroke? Sounds good to me!

> This brings me back to my point above, what real value are we getting if we don't strictly enforce the schemas?

I agree; without being spec-compliant the API has greatly reduced value to callers. The XML guys will fail validation, the JSON guys will tear their hair out trying to figure out why the docs are wrong.

I'm proposing we incorporate the schema so that we can fix up the API. I'd love to enforce it strictly so that everyone _had_ to fix their APIs, but we can't do that in this development process. You're free to tweak the flags and tell your team to get on it though.

> It isn't a validation argument Brian (L!). My logic is that one
> should derive from the other, so that we don't have to double our
> workload and can focus on just getting it right.

I believe you are talking about the same thing--there is no confusion about validation.

In my view, the XML and JSON representations are both first class citizens that can evolve independently according to their own needs.

I am actually more concerned about the development effort of maintaining a system that unifies the XML and JSON. Inevitably it will either be too inflexible for our needs or too fragile to easily maintain.

> I think treating XML and JSON separately comes last in my ranking list.

Still tops my list.

In JavaLand, this is essentially the standard technique now (and it is
really great). I think it's the same in RubyVille. Hopefully this
will make it into Python as a core library sometime soon, so you won't
have to maintain it.

Until that happens, I think the best thing to do here is to agree to
disagree :-) I don't think it's necessarily relevant to the merge
proposals?

> Do you mean using strongly typed domain objects, so that we wipe out half our
> bugs in a single stroke? Sounds good to me!

Actually, yes. Our team has discussed this quite a bit and we will probably look at it for the next release. Maybe this is something to talk about at the conference?

> I don't think it's necessarily relevant to the merge proposals?

You're right, but we still have to come to an agreement on *this* merge prop. I'm not against the idea, but I think we should wait until we address serialization (https://blueprints.launchpad.net/nova/+spec/nova-api-serialization) for Diablo. I dont want you to put a lot of work into this then have it get dropped.

> Actually, yes. Our team has discussed this quite a bit and we will probably look at it for the next release. Maybe this is something to talk about at the conference?

I've proposed a design summit discussion on what I call "engineering
in quality"..
https://blueprints.launchpad.net/nova/+spec/engineer-in-quality

The usual factions aligned themselves against it, but hopefully we can
have the discussion. Feel free to add to the blueprint!

> ..we still have to come to an agreement on *this* merge prop. I'm not against the idea, but I think we should wait until we address serialization (https://blueprints.launchpad.net/nova/+spec/nova-api-serialization) for Diablo. I dont want you to put a lot of work into this then have it get dropped

On Thu, Apr 7, 2011 at 2:16 PM, justinsb <email address hidden> wrote:
>> Actually, yes. Our team has discussed this quite a bit and we will probably look at it for the next release. Maybe this is something to talk about at the conference?
>
> I've proposed a design summit discussion on what I call "engineering
> in quality"..
> https://blueprints.launchpad.net/nova/+spec/engineer-in-quality
>
> The usual factions aligned themselves against it, but hopefully we can
> have the discussion.  Feel free to add to the blueprint!

The usual factions? If you want adult conversations and discussions at
the summit, please stop the snarky comments like this. They don't
help.

-jay

Jay, I don't think I was unfair. I didn't mean to criticize, and nor
did I. You and a few others have a very particular viewpoint on what
I would call 'traditional' software engineering practice, and so I
look forward to the discussion to see if we can get the best of both
worlds.

Save your ammo for the git / bazaar showdown :-)

On Thu, Apr 7, 2011 at 2:49 PM, Justin Santa Barbara
<email address hidden> wrote:
> Jay, I don't think I was unfair.  I didn't mean to criticize, and nor
> did I.  You and a few others have a very particular viewpoint on what
> I would call 'traditional' software engineering practice, and so I
> look forward to the discussion to see if we can get the best of both
> worlds.

I look forward to the discussion as well if the discussion can be kept
at an adult level.

> Save your ammo for the git / bazaar showdown :-)

There won't be one.

-jay

I may be missing a couple things due to the diff/prereq business, but in general I think the gist of what you're talking about is generating XML for every JSON response, validating the XML according to the predefined schemas, and then returning JSON.

I think that is an interesting idea for sure though, as I think you mentioned, I'd only expect it to come into play in testing / devel and this doesn't look like a testing specific patch. I would probably put a flag in 'api_always_validate_xml' or something of that sort and then we can start passing that in when running tests.

This hasn't been touched since before the summit, should it be set to work in progress?