#metabrainz

MBS-8393: Extend dynamic attributes to all entities

reosarevok, yvanzo : where does the main rendering file sit ? I mean the one with the headers

mo’’in’

Cyna: see root/layout

SothoTalKer: development suspended for months, should get back to it after schema change 2019 Q2 features

yvanzo: I mean the index.html type of file

root/layout.tt file when rendering Perl templates, root/layout/index.js when rendering JS templates

homepage is root/main/index.js

spellew: I think it makes sense to keep it in CB. Why: in CB we can't directly use functions in entities.py if they are present in BU. For example, `get_multiple_entities` fetches data for multiple mbids of different primary entities. get_primary_entities would call specific functions for each entity and we would very much want to get the cached versions to avoid db queries.

where is the index.html file which usually includes headers where the script for jqueryUi at client exist

Cyna: there is no index.html file, jQuery UI is included in root/static/scripts/common.js which is included in root/layout/components/Head.js (which is included i both root/layout.tt and root/layout/index.js mentioned above)

Mr_Monkey: hi

[musicbrainz-server] reosarevok merged pull request #1096 (master…MBS-10217): MBS-10217: Explain AC renaming in artist/edit_form https://github.com/metabrainz/musicbrainz-serve...

MBS-10217: Explain what renaming artist credits does when editing artist https://tickets.metabrainz.org/browse/MBS-10217

I want to checkout the version of jQueryUI used

Mr_Monkey: yesterday I pushed some refactor code. But tests are not passed at CI. I was tried the solution you mentioned. Please look it once.

yvanzo: the jquery ui version must be above 1.20.0 as there is a bug while using react with it

You can check this guys comment in there

Hi akhilesh ! AS far as I can see, you refactored to use expect instead of should. I think it's a good thing, but that's not the solution that was suggested in the github issue comment I sent yesterday.

The fix suggested was to use a try-catch block around your `await chai.request(app).get(…` promise calls, and do your test in the catch block.

This is equivalent to saying 'I expect this code to throw an error, and I expect the error to be code XXX and message YYY'

Try that out and see if that helps the CI

I was also tried that solution, but it was failing at my system.

May you try for one test please, if it will work then I will refactor whole. Mr_Monkey

OK, let me try

Mr_Monkey: Can you suggest please, a better err handling instead of direct return response at line 14 and 17? https://github.com/bookbrainz/bookbrainz-site/p...

akhilesh: What is the issue with how it is currently done?

Here's how the BB site does it: https://github.com/bookbrainz/bookbrainz-site/b...

akhilesh: After some research, this commit has one 404 test I modified to pass on the CI server. You can apply the same logic for the other tests.

The trick is to use the callback form chai-http`chai.get(…).end(error, response)` instead of using promises (either `chait.get(…).then(response).catch(error)` or using a try…catch block around `await chai.get(…)` ).

That also requires we use the callback form for the mocha test itself, and manually call `done()` (don't forget to pass the done callback in the test: `it('should do something', function (done) {…`)

Sorry, forgot to paste the commit: https://github.com/bookbrainz/bookbrainz-site/p...

That one test I modified is now passing both locally and on the CI server

Ok, thanks Mr_Monkey !

No problem !

Mr_Monkey: Should I use same logic for all tests? Or only on, which was failing at CI?

Only the ones where you need to test an error response. Any valid response (200) you can test 'normally'

Ok

akhilesh: To answer your earlier question, I think the error handling in `makeEntityLoader` is fine as it is. I tested manually (with postman), and I'm getting the correct errors and messages.

Ok

akhilesh: Postman is a nice tool to test the api: https://www.getpostman.com/

Mr_Monkey: please suggest some more test cases for existing endpoint. What kind of test, I can implement for strong testing?

Mr_Monkey: I used postman previously, that is too good.

Yes, that was my next step, let's combine efforts to think about as many use cases as possible

SothoTalKer: some of it coded, but it needs implementation and fixingbecause h code was a while a go

reosarevok: knows more

Oh no

Oh. yvanzo knows more than I do, obviously :)

But yeah, the SQL is already in, there's unfinished code implementation but it probably needs updating

yvanzo: are multiple test URLs possible?

SothoTalKer: Just add multiple entries/tests.

Look at how it’s done for other cleanups.

Mr_Monkey: hi

> Authors obviously have considerable experience with the creation of large-scale and *open* MIR datasets. While any dataset can be criticized for some feature (or absence thereof), this dataset is brilliant for having all the pieces readily accessible notwithstanding the different sources. The genre problem represented in this paper is also nicely considered (i.e., multi-label, variant, etc.).

we just had a paper accepted which contains ~1m feature files from AcousticBrainz along with genre annotations from 4 different sites for use in automated genre classification algorithms

[acousticbrainz-server] aidanlw17 opened pull request #347 (master…select-features): AB-404: Bulk endpoint for select lowlevel features https://github.com/metabrainz/acousticbrainz-se...

AB-404: Provide an API endpoint where users can select only the features that they want returned https://tickets.metabrainz.org/browse/AB-404

some nice feedback. thanks to everyone who added good data to it!

alastairp: check out that PR when you get the chance - I meant to put it in draft mode but I don't think I can go back now

I have a couple questions for you which I'll attach as comments

And I need to build unit tests as well.

it's OK that it's not a draft

(tests should be written in-line with the work!)

I'm trying really hard in my work to write the test as I write the functionality

it's pretty difficult, but satisfying when it works well

Ok! I've read a bit about test driven dev, even writing them first - I'll try to shift my approach

btw, I made AB-410 too, which is the same for highlevel

AB-410: Provide an API endpoint to select only highlevel models that are wanted https://tickets.metabrainz.org/browse/AB-410

OK, there's quite a bit of feedback, I'll try and get to it on the weekend

Ok thanks alastairp, I mainly was having a tough time with passing the feature paths into the postgres query.

yeah! that's a bit complex

I think what you did is basically the best way to do it

Yeah, especially because I wanted them all to be in a single query

It just got a bit hard to follow

sqlalchemy has a feature where you can write queries using a syntax that looks like class methods. we don't use it, but I like it for things like this because you can write a loop that generates these types of computational queries

Yeah I've used the class method syntax with sqlalchemy before, do you choose to write the raw queries for speed or other reasons?

not for speed, just to remove complexity.

also most of us in the musicbrainz ecosystem are more used to writing sql

it allows us to write a specific query and get all the data that we want

instead of having to work around limitations of the ORM implementation

Okay I see. In most cases I find the raw query is clearer to work with

this is ince

nice

you could see if there is a way to use this class to generate the queries, but I think that it'll be a bit difficult if we're not already using it

Index operations returning text (the ->> operator):

data_table.c.data['some key'].astext == 'some value'

Yeah I can look into doing it that way, it might clean things up a bit

When I was writing the query, using the `:features` syntax then passing the parameters as a dict for the second argument of `connection.execute(query, {"features": features})`, I wasn't able to get the correct paths because they were processed with extra double quotes around them

yeah, that's correct

So they looked like `SELECT "llj.data->'feature'"` rather than `SELECT llj.data->'feature'`

the parameters for .execute are to escape and quote user input

Okay, so that's why you need to use the placeholder %s and write as "select %(feature)s . ..." % feature instead?

right

but be really careful with that query (I'm about to write a comment on it)

you should _never_ pass in user-provided data with string formatting

For sql injection security?

and you're passing in the recording ids, this could potentially cause a security vulnerability

yes

so we have to use string formatting for the columns that we return, but use the params argument to execute for the recording ids

I'm not sure I follow what you just said

you're using string formatting here to add recordings

Oh I now see what you mean

So I need to use params argument when I pass in the recordings

Makes sense

But, how come the same doesn't follow for where I add the features in?

because they're different parts of the sql query

So the danger isn

SELECT field FROM database WHERE field = :value

*the danger for injection isn't in the select part of the query?

the djanger for injection is that you directly add user-provided data into the sql query

it's just that 99% of the time that people write queries that could be exploited, it's because the user-provided data is added to the WHERE

keep in mind also that we're not passing a user parameter directly into the query

we're taking a parameter and using that to generate some sql ourselves

Yeah so when we add user-provided features with string formatting, the danger is less because we've also parsed the features