Fixing duplicate sources bug when creating claim

[LuizFNJ]

Description

I debugged the process of creating a claim using both existing and non-existing sources, and I noticed that the conditional meant to determine whether to create or update the sources was always returning true, causing it to always try to create the source even when it already existed in the database, thus generating the error.

Claim created for testing with 3 sources already existing on the site and 3 new ones:

Before creating the claim:

After, adding the target ids to them:

And creating the new ones:

I created a new function to search for the href values inside the source database, and I called this function with the source value received during the claim creation. This way, it returned an array of hrefs that already exist in the database, stored in the existingSource constant. Then, I added a check to see if any value was returned. If any value was found, it would execute the update function, thus updating the source. If it returned an empty array, nothing would be updated, and the source would proceed to the else where it would be created as a new source.

Fixes #1796

Type of change

• Bug fix (non-breaking change which fixes an issue)

Testing

To test this, you should create claims with existing sources on the site and new sources to test this functionality.

Developer Checklist

General

• Code is appropriately commented, particularly in hard-to-understand areas
• Repository documentation has been updated (Readme.md) with additional steps required for a local environment setup.
• No console.log or related logging is added.
• No code is repeated/duplicated in violation of DRY. The exception to this is for new (MVP/Prototype) functionality where the abstraction layer may not be clear (comments should be added to explain the violation of DRY in these scenarios).
• Documented with TSDoc all library and controller new functions

Frontend Changes

• No new styling is added through CSS files (Unless it's a bugfix/hotfix)
• All types are added correctly

Backend Changes

• All endpoints are appropriately secured with Middleware authentication
• All new endpoints have a interface schema defined

Tests

• All existing unit and end to end tests pass across all services
• Unit and end to end tests have been added to ensure backend APIs behave as expected

Merge Request Review Checklist

• An issue is linked to this PR and these changes meet the requirements outlined in the linked issue(s)
• High risk and core workflows have been tested and verified in a local environment.
• Enhancements or opportunities to improve performance, stability, security or code readability have been noted and documented in JIRA issues if not being addressed.
• Any dependent changes have been merged and published in downstream modules
• Changes to multiple services can be deployed in parallel and independently. If not, changes should be broken out into separate merge requests and deployed in order.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[pepermao]

nitpick: consider adding the await keyword in future opportunities

[caneppelevitor]

Awaiting updateTargetId is unnecessary here because its result isn't used, and it's the last operation in this branch, so it doesn't block further processing.

[pepermao]

The try/catch block won't catch possible errors when updating the source target ID if we don't await the response. Following your conclusion, we don't need to await to save documents in the db, since it's the last operation and the result isn't used. However the success or failure status are used in both cases

[caneppelevitor]

Errors will still be caught by the try/catch block, even without await, since promise rejections propagate to the catch.

[pepermao]

Well, not sure about that. could you send me the documentation for that ?

[pepermao]

This is a very simple example where the console "save in the database" occurs even though we got an error in the db transaction. The promise execution starts but does not throw an error inside the try block because it's asynchronous. Any errors will be unhandled unless explicitly caught later.

async function test() {
    return new Promise((resolve, reject) => {
        setTimeout(() => {
            console.log('test')
            reject(new Error("error"))
        }, 2000)
    })
}

async function test2() {
    try {
        test()
        console.log("save in the database")
    } catch (e) {
        console.log("ERROR: ", e.message)
    }
}

test2()

// save in the database
// undefined
// VM744:3 test
// VM744:4 Uncaught Error: error
//  at <anonymous>:4:15

[pepermao]

https://javascript.info/async-await

[caneppelevitor]

After looking into this more deeply, I agree that await should be used for proper error handling. Using await ensures that if the underlying function—like updateTargetId—throws or returns a rejected Promise, the error will be caught in the same try/catch block. However, this only works if the function itself actually propagates errors by awaiting its own database calls. If, for instance, we never await source.save() inside updateTargetId, then a DB error in the save step won’t bubble up, even if we do await updateTargetId in the calling code. So, to fully capture errors, both the function and the caller need to consistently use await.

[pepermao]

Nice, could you provide the documentation that you looked at ? I think it's worth to share with everybody else. Thanks for the discussion, it's always good to have these kind of talks, I almost forgot about the try/catch error handling concept