search

balanced / balanced-api

Balanced API specification.
220 stars 72 forks source link

Regression in `/events` #591

Closed steveklabnik closed 10 years ago

steveklabnik commented 10 years ago

@matthewfl and @cieplak , I believe you two introduced a regression:

$ curl https://api.balancedpayments.com/events       -H "Accept: application/vnd.api+json;revision=1.1"       -uak-test-xJZVlcgNMuLGg0Qf5zEx600qDhYI1LZi:

This, being a freshly created marketplace, has no events:

{
  "meta": {
    "last": "/events?limit=10&offset=0",
    "next": null,
    "href": "/events?limit=10&offset=0",
    "limit": 10,
    "offset": 0,
    "previous": null,
    "total": 0,
    "first": "/events?limit=10&offset=0"
  },
  "links": {}
}

This fails the test because the schema assumes that an events key will be there, but since there are none, it gets dropped entirely.

Now, the spec isn't actually clear if this is truly required: https://github.com/json-api/json-api/issues/226

I would like for us to always return it, though.

matthewfl commented 10 years ago

https://github.com/balanced/balanced-api/blob/master/fixtures/events.json#L17

Even if we return an empty events array the spec will still fail

What needs to happen is that the spec needs to poll for events as they will take time to propagate. We are already doing this for disputes, so it should not be that hard to add for events as well.

steveklabnik commented 10 years ago

We shouldn't ever get a response that's invalid, even if it's before they propagate.

matthewfl commented 10 years ago

as you noted on the jsonapi repo, it is not clear what the behaviour was for this case, so there is no "invalid" response afaik on this point

steveklabnik commented 10 years ago

Yes, but it's invalid as to what we currently promised our customers, hence the failing test. I'm not entirely opposed to changing it, I just want to make sure that we're not running red.

steveklabnik commented 10 years ago

Apparently this is @mjallday 's fault, not @matthewfl and @cieplak 's.

steveklabnik commented 10 years ago

Wow, with the 'allow zeros' schema change I'm straight-up getting 500s sometimes:

SECRET: ak-test-KAoMvZKYQNarjJq9Pbs86cCJW1uEhM7h
MARKETPLACE: TEST-MP3o0AT6OL1MNuqP5MxonE9z
.F-

(::) failed steps (::)

757: unexpected token at '<html>
<head><title>504 Gateway Time-out</title></head>
<body bgcolor="white">
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>nginx/1.4.7</center>
</body>
</html>
' (JSON::ParserError)
./lib/balanced/tiny_client.rb:140:in `last_body'
./features/step_definitions/http_steps.rb:150:in `/^I should get a (.+) status code$/'
features/rest/events.feature:9:in `Then I should get a 200 OK status code'
steveklabnik commented 10 years ago

This seemingly affects any end point where there isn't anything to return:

{"meta"=>{"last"=>"/cards?limit=10&offset=0", "next"=>nil, "href"=>"/cards?limit=10&offset=0", "limit"=>10, "offset"=>0, "previous"=>nil, "total"=>0, "first"=>"/cards?limit=10&offset=0"}, "links"=>{}}

That's /cards for someone who has deleted their only tokenized card.

This means this bug is causing the last two spec failures.

mjallday commented 10 years ago

Internal issue tracking this https://github.com/PoundPay/balanced/issues/257

matin commented 10 years ago

@matthewfl @mjallday Part of the goal for the scenarios is to prevent regression. It sounds like it accomplished that goal in this case.

How did code get pushed in the first place if it caused the scenarios?

mjallday commented 10 years ago

Apparently this is @mjallday's fault

Little more context would be nice. I'm not sure what I did, how I can fix it or...?! :cry:

steveklabnik commented 10 years ago

That's just what @matthewfl told me, I knew I shouldn't have trusted him! :P

matthewfl commented 10 years ago

I was referring to making events async. The solution to this is to make the spec poll for events, similar to what we are already doing for disputes

steveklabnik commented 10 years ago

This is being bumped back at least a week.

matin commented 10 years ago

What is the exact solution here?

Does something need to change in the scenarios or in the API?

/cc @mjallday @steveklabnik @matthewfl

mjallday commented 10 years ago

I believe we just need to always pass a collection even when it's empty.

@steveklabnik can correct me, this is what I believe the correct payload would look like (note the events collection)

{
  "meta": {
    "last": "/events?limit=10&offset=0",
    "next": null,
    "href": "/events?limit=10&offset=0",
    "limit": 10,
    "offset": 0,
    "previous": null,
    "total": 0,
    "first": "/events?limit=10&offset=0"
  },
  "links": {},
  "events": {}
}
steveklabnik commented 10 years ago

Yes, that is correct.

matin commented 10 years ago

@mjallday Could you (or someone else) fix that today or tomorrow?

mahmoudimus commented 10 years ago

Delete this spec if this is blocking anything.

matin commented 10 years ago

Why do that instead of fixing in the API?

mahmoudimus commented 10 years ago

Because this is not urgent. This endpoint and test is an edge case. Events are never empty in the real world.

matin commented 10 years ago

@steveklabnik Talked to @mjallday offline. This will take 1+ days to fix and doesn't produce any issues in practice. Can you remove the scenario that's causing this to fail?

steveklabnik commented 10 years ago

:disappointed:

Previously, we promised our customers certain behavior. We're now changing that because it's inconvenient.

(and frankly, it's pretty ridiculous that this should take more than ten minutes. I guess Ruby's just that much better than Python. :fire: :wink: )

mjallday commented 10 years ago

that last line got me really close to hacking in a fix for this. well played sir. </hattip>

mahmoudimus commented 10 years ago

ruby = hack hack hack python = engineer, test, ensure :)

matin commented 10 years ago

@steveklabnik I know where you're coming from.

This is where pragmatism comes in. Mahmoud is (correctly) arguing that this won't make a difference for any customers. I agree. The moment you've created a single object in the API the events response is no longer empty.

I'm not sure why it would take so long to fix, but I have to defer to @mjallday and @mahmoudimus on this. It's just not worth it. The time could be used to fix other issues in the API that are effecting customers today.

steveklabnik commented 10 years ago

I cannot in good faith tell our customers that we have a commitment to backwards compatibility if we don't have it. Other changes to Events have already had multiple customers call me out on this.

steveklabnik commented 10 years ago

(my claim about the simplicity of Ruby is fun joking, but it's also semi serious. I would add a || [] and be done with it. Five characters.)

And @mahmoudimus, if it was so well engineered, we wouldn't have a regression in the first place. :wink:

matin commented 10 years ago

@steveklabnik Do you think that this particular issue will effect any customers? Is my explanation of when this issue arrises accurate? If it is, then it shouldn't be a breaking change for any customers since they already have at least one event. It's not a breaking change for new customers since they haven't built around this yet.

steveklabnik commented 10 years ago

This issue? Probably not. This attitude towards compatibility? In my darker days, it makes me want to move to a farm somewhere. It's why we can't have nice things.

Like I said, we can totally make this change, that just means when our customers complain about interface changes, I'll tell them the truth: we're as compatible as we feel like being.

steveklabnik commented 10 years ago

Oh, and I only say "probably not" because I haven't been able to get out of customers what exactly the change was. But multiple customers have already complained to me about changes to events, so while in theory it shouldn't really hurt anything, theory and practice are different.

matin commented 10 years ago

Assumed it was deployed since you closed the issue :wink:

matin commented 10 years ago

@mjallday When do you expect for this fix to be deployed?

mjallday commented 10 years ago

Should already be deployed

curl https://api.balancedpayments.com/events/EVcbe325dad49611e387ac06ac84487d12/callbacks -H "Accept: application/vnd.api+json;revision=1.1" -uak-test-xJZVlcgNMuLGg0Qf5zEx600qDhYI1LZi:

{
  "event_callbacks": [],
  "meta": {
    "last": "/events/EVcbe325dad49611e387ac06ac84487d12/callbacks?limit=10&offset=0",
    "next": null,
    "href": "/events/EVcbe325dad49611e387ac06ac84487d12/callbacks?limit=10&offset=0",
    "limit": 10,
    "offset": 0,
    "previous": null,
    "total": 0,
    "first": "/events/EVcbe325dad49611e387ac06ac84487d12/callbacks?limit=10&offset=0"
  },
  "links": {}
}
steveklabnik commented 10 years ago

:heart_eyes: :heart: :confetti_ball: