#metabrainz

/

      • SothoTalKer
        yvanzo: what is the status of https://tickets.metabrainz.org/browse/MBS-8393 ?
      • BrainzBot
        MBS-8393: Extend dynamic attributes to all entities
      • Sophist-UK has quit
      • Sophist-UK joined the channel
      • Nyanko-sensei joined the channel
      • D4RK-PH0ENiX has quit
      • Sophist_UK joined the channel
      • Sophist-UK has quit
      • Sophist_UK has quit
      • Sophist-UK joined the channel
      • Nyanko-sensei has quit
      • D4RK-PH0ENiX joined the channel
      • Wizzup has quit
      • Sophist_UK joined the channel
      • Sophist-UK has quit
      • Wizzup joined the channel
      • Cyna
        reosarevok, yvanzo : where does the main rendering file sit ? I mean the one with the headers
      • Sophist_UK has quit
      • yvanzo
        mo’’in’
      • Cyna: see root/layout
      • SothoTalKer: development suspended for months, should get back to it after schema change 2019 Q2 features
      • alastairp has quit
      • Cyna
        yvanzo: I mean the index.html type of file
      • alastairp joined the channel
      • yvanzo
        root/layout.tt file when rendering Perl templates, root/layout/index.js when rendering JS templates
      • homepage is root/main/index.js
      • Nyanko-sensei joined the channel
      • D4RK-PH0ENiX has quit
      • Nyanko-sensei has quit
      • D4RK-PH0ENiX joined the channel
      • ferbncode
        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.
      • Cyna
        where is the index.html file which usually includes headers where the script for jqueryUi at client exist
      • yvanzo
        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)
      • chirlu has quit
      • chirlu joined the channel
      • akhilesh
        Mr_Monkey: hi
      • BrainzGit
        [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...
      • BrainzBot
        MBS-10217: Explain what renaming artist credits does when editing artist https://tickets.metabrainz.org/browse/MBS-10217
      • Cyna
        I want to checkout the version of jQueryUI used
      • dolina has quit
      • akhilesh
        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.
      • dolina joined the channel
      • Cyna
        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
      • gr0uch0mars joined the channel
      • Mr_Monkey
        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
      • akhilesh
        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
      • Mr_Monkey
        OK, let me try
      • akhilesh
        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...
      • Mr_Monkey
        akhilesh: What is the issue with how it is currently done?
      • D4RK-PH0ENiX has quit
      • D4RK-PH0ENiX joined the channel
      • D4RK-PH0ENiX has quit
      • D4RK-PH0ENiX joined the channel
      • 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
      • akhilesh
        Ok, thanks Mr_Monkey !
      • Mr_Monkey
        No problem !
      • akhilesh
        Mr_Monkey: Should I use same logic for all tests? Or only on, which was failing at CI?
      • Mr_Monkey
        Only the ones where you need to test an error response. Any valid response (200) you can test 'normally'
      • akhilesh
        Ok
      • Mr_Monkey
        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.
      • akhilesh
        Ok
      • Mr_Monkey
        akhilesh: Postman is a nice tool to test the api: https://www.getpostman.com/
      • akhilesh
        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.
      • Mr_Monkey
        Yes, that was my next step, let's combine efforts to think about as many use cases as possible
      • CatQuest
        SothoTalKer: some of it coded, but it needs implementation and fixingbecause h code was a while a go
      • reosarevok: knows more
      • reosarevok
        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
      • SothoTalKer
        yvanzo: are multiple test URLs possible?
      • gr0uch0mars has quit
      • Freso
        SothoTalKer: Just add multiple entries/tests.
      • Look at how it’s done for other cleanups.
      • alastairp
        Mr_Monkey: hi
      • Rotab has quit
      • > 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
      • BrainzGit
        [acousticbrainz-server] aidanlw17 opened pull request #347 (master…select-features): AB-404: Bulk endpoint for select lowlevel features https://github.com/metabrainz/acousticbrainz-se...
      • BrainzBot
        AB-404: Provide an API endpoint where users can select only the features that they want returned https://tickets.metabrainz.org/browse/AB-404
      • alastairp
        some nice feedback. thanks to everyone who added good data to it!
      • aidanlw17
        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.
      • alastairp
        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
      • aidanlw17
        Ok! I've read a bit about test driven dev, even writing them first - I'll try to shift my approach
      • alastairp
        btw, I made AB-410 too, which is the same for highlevel
      • BrainzBot
        AB-410: Provide an API endpoint to select only highlevel models that are wanted https://tickets.metabrainz.org/browse/AB-410
      • alastairp
        OK, there's quite a bit of feedback, I'll try and get to it on the weekend
      • aidanlw17
        Ok thanks alastairp, I mainly was having a tough time with passing the feature paths into the postgres query.
      • alastairp
        yeah! that's a bit complex
      • I think what you did is basically the best way to do it
      • aidanlw17
        Yeah, especially because I wanted them all to be in a single query
      • It just got a bit hard to follow
      • alastairp
        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
      • aidanlw17
        Yeah I've used the class method syntax with sqlalchemy before, do you choose to write the raw queries for speed or other reasons?
      • alastairp
        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
      • aidanlw17
        Okay I see. In most cases I find the raw query is clearer to work with
      • alastairp
      • 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'
      • aidanlw17
        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
      • alastairp
        yeah, that's correct
      • aidanlw17
        So they looked like `SELECT "llj.data->'feature'"` rather than `SELECT llj.data->'feature'`
      • alastairp
        the parameters for .execute are to escape and quote user input
      • aidanlw17
        Okay, so that's why you need to use the placeholder %s and write as "select %(feature)s . ..." % feature instead?
      • alastairp
        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
      • aidanlw17
        For sql injection security?
      • alastairp
        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
      • aidanlw17
        I'm not sure I follow what you just said
      • alastairp
      • you're using string formatting here to add recordings
      • aidanlw17
        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?
      • alastairp
        because they're different parts of the sql query
      • aidanlw17
        So the danger isn
      • alastairp
        SELECT field FROM database WHERE field = :value
      • aidanlw17
        *the danger for injection isn't in the select part of the query?
      • alastairp
        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
      • aidanlw17
        Yeah so when we add user-provided features with string formatting, the danger is less because we've also parsed the features