919710 - published needs to be enforced

Status: RESOLVED DUPLICATE of bug 922166

(Webmaker Graveyard :: MakeAPI, defect)

Description

[Scott [:thecount] Downe]

I'm working on the front end of allowing projects to be saved privatly, then later published in bug 913731.

In order for this to mean anything, we need it to be enforced from inside the MakeAPI.

We're going to limit the public search to never return isPublished: false just like it doesn't return deleted makes.

Then we need an authenticated search. I'm thinking the authenticated search can be a new ticket.

Comment 1

[Scott [:thecount] Downe]

Attachment: https://github.com/mozilla/MakeAPI/pull/151

Here it is.

No review request yet, as I'm still testing it.

It's not easy to test, so I'm basically going to implement the other side in popcorn maker. Once both patches are working together, I'll put this in review.

The current solution to this is expose an authenticatedSearch for apps to call to get un published and deleted makes.

Comment 2

[Scott [:thecount] Downe]

Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Probably easiest to test with bug 913731 as that allows you to publish a make to the makeapi with published === false, by hitting just save, then try to load that make again and it should be filtered out.

This one blocks bug 913731 though so obviously bug 913731 cannot land for multiple reasons.

Comment 3

[Scott [:thecount] Downe]

Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Adding a second reviewer because Chris is fairly busy.

Comment 4

[Matthew Schranz [:mjschranz]]

Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Logic wise, this appears to be correct. I was able get some testing by saving projects in your save/publish PM patch before using this patch (saw them in a search for my username) and then with it (they were not present in a search for my username).

Minor nits, but I'll let Chris get back to this.

Comment 5

[Chris DeCairos (:cade)]

Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Requests that are made with Hawk must also have their responses signed with hawk so that the requester can verify the responses' authenticity and integrity.

Should you write tests for the changes you've made? (custom filters passed into the build function)

Comment 6

[Scott [:thecount] Downe]

Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

I moved authenticated search out, going to do that in another ticket while I figure out what hawk is actually doing.

Keeping this ticket limited to just stripping published via a filter.

We already have tests for the filter we support. I would say write tests for what we support. Once we've added a new filter, we should create a test for it. It's not exposed to anyone on the other end of the call, just internal.

I wouldn't mind a test for making sure a non published project doesn't get returned.

Comment 7

[Chris DeCairos (:cade)]

Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

r+ with one comment for you to consider in the Pull Request.

Comment 8

[Scott [:thecount] Downe]

Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

Agreed.

Comment 9

[Chris DeCairos (:cade)]

Comment on attachment 809464 [details] [review]
https://github.com/mozilla/MakeAPI/pull/151

r+ here.

Comment 10

[Scott [:thecount] Downe]

Staged: https://github.com/mozilla/MakeAPI/commit/e8207c8273f6cd64475bc9f9bf7f329f3fa8606e

Needs verification.

Comment 11

[Scott [:thecount] Downe]

This got backed out because it didn't work on staging.

Needs some kind of migration script to fix the published property.

Comment 12

[Brett Gaylor [:brett]]

moving from mozfest milestone

Comment 13

[Matthew Schranz [:mjschranz]]

At this point there doesn't seem to be any "clean" option to fixing this and we might need to do something by hand.

I believe I can change the mapping manually and it would work but I can't confirm since I can't reproduce the problem locally. The other solution is to basically going to be create a copy of the current data, make new mappings with the correct data and run a script to update all records because old projects have published set to false.

Here's a gist of all the mapping differences I have pulled locally https://gist.github.com/mjschranz/910b0023ef11b833caa0

Jon, I need your help to get this done. Is there a time we can set aside to figure this out and do it?

Comment 14

[Jon Buckley [:jbuck]]

How can you change the mapping manually? Is there a command line script or app you can run to change the indices?

Comment 15

[Matthew Schranz [:mjschranz]]

Basically they have this PUT mapping API. Their examples of it use a CURL request and I tried it locally just playing around with my existing mapping and it seemed okay.

Chris seems to think this on it's own will not work but I have no way of knowing for sure since the problem can't be replicated locally.

Comment 16

[Matthew Schranz [:mjschranz]]

Derp, forgot the link. http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/indices-put-mapping.html

Comment 17

[Jon Buckley [:jbuck]]

Okay, after doing a bunch of research and reading http://www.elasticsearch.org/blog/changing-mapping-with-zero-downtime/ I think we should just add a new mapping that isn't named 'published' and forget that 'published' ever existed.

My reasoning here is:
1) You can't delete a mapping(!)
2) We don't care about the data currently stored in the 'published' field, it's currently true for all makes
3) We can't upgrade to a multi-field, because published is currently an "object" type
4) Doing schema migration using aliases as described in the blog post looks amazing, but I think this will require much more setup and planning. Might be worth looking at though!
5) Re-indexing the ES data from scratch in MongoDB will require downtime. Not sure how long, would need to test this.

Comment 18

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/makeapi-client/pull/18

Client change.

Comment 19

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/MakeAPI/pull/179

Probably will want to update the client version when that lands in this patch as well.

Comment 20

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/353

Popcorn Maker Patch.

Comment 21

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/goggles.webmaker.org/pull/74

Goggles Patch.

Comment 22

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/306

Thimble Patch!

Comment 23

[Matthew Schranz [:mjschranz]]

Comment on attachment 8340442 [details] [review]
https://github.com/mozilla/MakeAPI/pull/179

I derped and forgot I need a migration script.

Comment 24

[Matthew Schranz [:mjschranz]]

Comment on attachment 8340442 [details] [review]
https://github.com/mozilla/MakeAPI/pull/179

Added the script!

Comment 25

[Pomax]

I used the "published" flag in my https://bugzilla.mozilla.org/show_bug.cgi?id=912696 to separate save and publish for thimble projects. I've read this entire thread, but somewhere between comment 12 and comment 13 someone concluded that published cannot be used, without explaining why. I'm using it quite successfully in my Thimble patch, so I'd very much like to know why after comment 12 it suddenly became a problem.

Comment 26

[Chris DeCairos (:cade)]

Comment on attachment 8340442 [details] [review]
https://github.com/mozilla/MakeAPI/pull/179

Lets remove the changes to querybuilder and the tests. We'll not implement any logic that depends on the new field until we have UI for it (needs bug) and an authenticated search method for the my makes page (so that a user can manage their unlisted makes; Needs a bug)

Comment 27

[Pomax]

The missing rational: the published flag is supposed to be boolean, but is the wrong type on staging and production, without ES supporting an easy way to change the type. Using a new flag is a hack around this, I would like to recommend we instead fix the mistake we made when we deployed ES on staging and production and not tack on new flags. We're going to be maintaining this for (hopefully) a long time, polluting the schema like this is wilfully turning a blind eye early in the maintenance process. I'd much rather see us come up with a way to fix this ES issue on staging/production first, so that we have an established way to deal with it when we run into it in the future again. Which we almost certainly will. Let's do this right, and then once that's fixed, use the "published" flag in the patches.

Comment 28

[Pomax]

Comment on attachment 8340447 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/74

r- until it can be proven that we need to go down this road, rather than fixing the actual problem of ES having the wrong schema on staging and production.

Comment 29

[Pomax]

Comment on attachment 8340449 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/306

r- until it can be proven that we need to go down this road, rather than fixing the actual problem of ES having the wrong schema on staging and production.

Comment 30

[Matthew Schranz [:mjschranz]]

I'm still in the camp that doing these potential solutions is not worth it when we can simply add the new flag and removed the old unused field.

In anycase, the solutions Pomax want's are against things I do not have access to so I can't test this out myself. Putting on you Jon.

Comment 31

[Scott [:thecount] Downe]

I'm in the camp of until there is some sort of rush on this, we should look into doing it right.

Am I missing how the above patch removes the olf unused flag? I think the only problem here is the old flag is left to rot while we tack on a new one. Changing the name is kinda not important. If we can remove the old, and add a new, we have one way to do this right.

Comment 32

[Matthew Schranz [:mjschranz]]

Your right in that my patch doesn't actually remove that old flag. I think that would by and large accomplish what we need/what right now. I also think that when we get around to dropping Mongoosastic/Mongo this is a solution that can be handled then (or truthfully we could just manually delete the mappings).

I really would prefer to go with that solution in all honesty. Means we can get the work done faster and no downtime.

Comment 33

[David Humphrey (:humph)]

Both this and bug 944816 seem to be about issues with our ES deployments/indices, so I'd like to see us get a bug (maybe we have one already?) on dealing with those problems directly vs solving them in our apps or the MakeAPI.

Comment 34

[Pomax]

I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=945865 for us so we can figure out how to do elasticsearch migrations. I'd like to see us tackle that bug before we continue work on this bug.

Comment 36

[Jon Buckley [:jbuck]]

Assigning back to Matt because he did the work behind the patches. I still need to fix bug 944816 though, if only to add an alias to instead of directly accessing the index.

Comment 37

[Matthew Schranz [:mjschranz]]

Correct me if I'm wrong but we still need to wait on bug 944816? If so let's mark that as a blocker.

Comment 38

[Jon Buckley [:jbuck]]

Yep, it's set as a blocker

Comment 39

[Scott [:thecount] Downe]

So what's the plan on this once bug 944816 lands?

Is my initial backed out patch still valid?

https://github.com/mozilla/MakeAPI/commit/e8207c8273f6cd64475bc9f9bf7f329f3fa8606e

All that did was ensure makes that are private do not get returned from client side, and because right now every make is set as not private, everything should still be searchable.

bug 922166 makes it so server side can search for private makes. Then like bug 913731 every app is going to need some sort of UI or authenticated search patch.

Is this still the plan?

Comment 40

[Jon Buckley [:jbuck]]

Yep, that sounds correct.

Comment 41

[[github robot]]

Commit pushed to master at https://github.com/mozilla/MakeAPI

https://github.com/mozilla/MakeAPI/commit/c7a82cb4a75efa49f0ae4bdca39d9550d3b500a3
Bug 919710 - Fix published mapping

Comment 42

[Matthew Schranz [:mjschranz]]

So at this point I'm assuming we want patches for this bug to basically be:

- Ensuring normal searches don't return "private" makes
- Fix up all apps to set published false on initial create, and true on their respective publish steps.
- Also change the default of published to true.
- Ensure MakeAPI client doesn't need changes too.

Sound right?

Comment 43

[Jon Buckley [:jbuck]]

Yeah, there will be some sort of authenticated search path that will show unlisted makes, which is being done in bug 922166. I'm not certain how the published flag should be handled, I guess that depends on how each app handles that saved vs published distinction? We might want to default to published = true too.

Comment 44

[Matthew Schranz [:mjschranz]]

Actually it's already set to default to true and that seems to be where we want it for now. This patch basically seems to be just now the first bullet point.

Comment 45

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/MakeAPI/pull/189

Comment 46

[Jon Buckley [:jbuck]]

Comment on attachment 8366178 [details] [review]
https://github.com/mozilla/MakeAPI/pull/189

There's some tests that need to be fixed up too

Comment 47

[Matthew Schranz [:mjschranz]]

Comment on attachment 8366178 [details] [review]
https://github.com/mozilla/MakeAPI/pull/189

Ooops, forgot to bring those fixes over too from the previous patch.

Comment 48

[Chris DeCairos (:cade)]

Comment on attachment 8366178 [details] [review]
https://github.com/mozilla/MakeAPI/pull/189

This doesn't need to happen here - it'll happen in Bug 922166

Comment 49

[Matthew Schranz [:mjschranz]]

Well feel free to close this bug then.