New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression in `/events` #591

Closed
steveklabnik opened this Issue May 5, 2014 · 34 comments

Comments

Projects
None yet
5 participants
@steveklabnik
Contributor

steveklabnik commented May 5, 2014

@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: json-api/json-api#226

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

@matthewfl

This comment has been minimized.

Show comment
Hide comment
@matthewfl

matthewfl May 5, 2014

Contributor

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.

Contributor

matthewfl commented May 5, 2014

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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 5, 2014

Contributor

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

Contributor

steveklabnik commented May 5, 2014

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

steveklabnik referenced this issue May 5, 2014

Remove requirement for events to exist.
Sometimes, there will be zero events.
@matthewfl

This comment has been minimized.

Show comment
Hide comment
@matthewfl

matthewfl May 5, 2014

Contributor

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

Contributor

matthewfl commented May 5, 2014

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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 5, 2014

Contributor

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.

Contributor

steveklabnik commented May 5, 2014

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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 5, 2014

Contributor

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

Contributor

steveklabnik commented May 5, 2014

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

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 5, 2014

Contributor

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'
Contributor

steveklabnik commented May 5, 2014

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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 5, 2014

Contributor

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.

Contributor

steveklabnik commented May 5, 2014

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

This comment has been minimized.

Show comment
Hide comment
@mjallday

mjallday May 5, 2014

Contributor
Contributor

mjallday commented May 5, 2014

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 5, 2014

Member

@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?

Member

matin commented May 5, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@mjallday

mjallday May 6, 2014

Contributor

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...?! 😢

Contributor

mjallday commented May 6, 2014

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...?! 😢

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 6, 2014

Contributor

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

Contributor

steveklabnik commented May 6, 2014

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

@matthewfl

This comment has been minimized.

Show comment
Hide comment
@matthewfl

matthewfl May 6, 2014

Contributor

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

Contributor

matthewfl commented May 6, 2014

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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 6, 2014

Contributor

This is being bumped back at least a week.

Contributor

steveklabnik commented May 6, 2014

This is being bumped back at least a week.

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 14, 2014

Member

What is the exact solution here?

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

/cc @mjallday @steveklabnik @matthewfl

Member

matin commented May 14, 2014

What is the exact solution here?

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

/cc @mjallday @steveklabnik @matthewfl

@mjallday

This comment has been minimized.

Show comment
Hide comment
@mjallday

mjallday May 14, 2014

Contributor

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": {}
}
Contributor

mjallday commented May 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 14, 2014

Contributor

Yes, that is correct.

Contributor

steveklabnik commented May 14, 2014

Yes, that is correct.

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 14, 2014

Member

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

Member

matin commented May 14, 2014

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

@mahmoudimus

This comment has been minimized.

Show comment
Hide comment
@mahmoudimus

mahmoudimus May 14, 2014

Contributor

Delete this spec if this is blocking anything.

Contributor

mahmoudimus commented May 14, 2014

Delete this spec if this is blocking anything.

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 14, 2014

Member

Why do that instead of fixing in the API?

Member

matin commented May 14, 2014

Why do that instead of fixing in the API?

@mahmoudimus

This comment has been minimized.

Show comment
Hide comment
@mahmoudimus

mahmoudimus May 14, 2014

Contributor

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

Contributor

mahmoudimus commented May 14, 2014

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

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 14, 2014

Member

@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?

Member

matin commented May 14, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 14, 2014

Contributor

😞

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. 🔥 😉 )

Contributor

steveklabnik commented May 14, 2014

😞

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. 🔥 😉 )

@mjallday

This comment has been minimized.

Show comment
Hide comment
@mjallday

mjallday May 14, 2014

Contributor

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

Contributor

mjallday commented May 14, 2014

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

@mahmoudimus

This comment has been minimized.

Show comment
Hide comment
@mahmoudimus

mahmoudimus May 14, 2014

Contributor

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

Contributor

mahmoudimus commented May 14, 2014

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

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 14, 2014

Member

@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.

Member

matin commented May 14, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 14, 2014

Contributor

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.

Contributor

steveklabnik commented May 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 14, 2014

Contributor

(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. 😉

Contributor

steveklabnik commented May 14, 2014

(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. 😉

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 14, 2014

Member

@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.

Member

matin commented May 14, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 14, 2014

Contributor

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.

Contributor

steveklabnik commented May 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 14, 2014

Contributor

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.

Contributor

steveklabnik commented May 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 15, 2014

Member

Assumed it was deployed since you closed the issue 😉

Member

matin commented May 15, 2014

Assumed it was deployed since you closed the issue 😉

@matin

This comment has been minimized.

Show comment
Hide comment
@matin

matin May 19, 2014

Member

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

Member

matin commented May 19, 2014

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

@mjallday

This comment has been minimized.

Show comment
Hide comment
@mjallday

mjallday May 19, 2014

Contributor

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": {}
}
Contributor

mjallday commented May 19, 2014

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": {}
}

@mjallday mjallday closed this May 19, 2014

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik May 19, 2014

Contributor

😍 ❤️ 🎊

Contributor

steveklabnik commented May 19, 2014

😍 ❤️ 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment