Let’s Roast: GetUserProfile() Controller In Go Lang

One day I was given the task of “making the app work” and the app in question was a legacy system. I’m not sure what the people expect I would do with such things in order to magically make them “work”. If a code base is crap, it is crap. There is no other way to see it. Just like a war crime is a war crime no matter which camera angle you use.

In any case, this is one of those instances where I was reviewing the code base and although I did find pretty terrible stuff in there, I picked just this one controller function since there is a lot to learn from it.

And to the poor guy who wrote it … don’t read any further as you would definitely not like it.

A Little Intro To Context

This code is written in Golang. It is a microservice architecture and the function we are talking about resides within the User Management microservice naturally. Not a fan of Golang? Me neither, but work calls for me to fix the mess so I do it. No questions asked.

The framework used here is Gin Web Framework which is a very lightweight and bare minimum framework. Think of it as Express.js or Flask if you are familiar with either of them.

As I said, GetUserProfile function is actually a controller attached to a route named /profile/:clerk_id accessible by GET method. The :clerk_id part signifies that you can pass in any ID within the URL like /profile/my-clerk-id-123 and it will be accessible inside the controller function.

Clerk, if you don’t already know, is a third-party user authentication system. You can use it to authenticate the users without actually managing passwords and sessions in your own database. Clerk provides a unique string based ID for each user which in this case we are calling as clerk_id. This ID usually looks like a UUID e.g. bf53a7c6-bf07-49ff-9ae4-50e4fadd9c57.

In this post you will find me using the word gorm, which is simply an ORM used by Gin.

One important thing to note here is that the controller works just fine and adheres to the frontend’s requirements. How it manages to do that is an entirely different story.

That’s it for the context. Let the roast begin!

The Code In Question

// GetUserProfile godoc
// @Summary Get user profile by clerk ID
// @Description Retrieves the profile information for a user by clerk ID including company details
// @Tags users3ec
// @Accept json
// @Produce json
// @Param clerk_id path string true "Clerk ID"
// @Security BearerAuth
// @Success 200 {object} UserProfileResponse
// @Failure 400 {object} ErrorResponse "Bad request - invalid or missing clerk ID"
// @Failure 401 {object} ErrorResponse "Unauthorized - invalid or missing token"
// @Failure 404 {object} ErrorResponse "User not found"
// @Failure 500 {object} ErrorResponse "Internal server error"
// @Router /profile/{clerk_id} [get]
func GetUserProfile(c *gin.Context) {
	clerkID := c.Param("clerk_id")
	if clerkID == "" {
		c.JSON(http.StatusBadRequest, ErrorResponse{
			Error: "Clerk ID is required",
		})
		return
	}

	db := GetCompanyDBFromContext(c)

	logger.GetLogger().Infof("GetUserProfile Clerk ID: %v", clerkID)

	var user models.User
	if err := db.Where("clerk_id = ?", clerkID).Take(&user).Error; err != nil {
		logger.GetLogger().Errorf("Failed to retrieve user profile: %v", err)
		c.JSON(http.StatusNotFound, ErrorResponse{
			Error: "User not found",
		})
		return
	}

	number := user.Number
	if number == "" {
		number = "nan"
	}

	var company CompanyDetails
	companyQuery := "SELECT onboarding_id, company_name, industry_name, company_size, website_url FROM public.onboarding_table WHERE portal_id = ?"
	if err := db.Raw(companyQuery, user.CompanyID).Scan(&company).Error; err != nil {
		logger.GetLogger().Errorf("Failed to retrieve company information: %v", err)
		company = CompanyDetails{
			CompanyName:  "Unknown",
			IndustryName: "Unknown",
			CompanySize:  "Unknown",
			WebsiteURL:   "",
		}
	}

	userDetails := UserDetails{
		Number:              number,
		ClerkID:             user.ClerkID,
		ReferralCode:        user.ReferralCode,
		ConnectAccounStatus: user.ConnectAccounStatus,
		PayoutsEnabled:      user.PayoutsEnabled,
		DetailsSubmitted:    user.DetailsSubmitted,
		StripeObjections:    user.StripeObjections,
	}

	c.JSON(http.StatusOK, UserProfileResponse{
		User:    userDetails,
		Company: company,
	})
}

Dry Running

Before we start applying the heat, let's briefly walk through the seven stages of the original GetUserProfile function to understand its intent and flow.

We begin with the Gin controller context c *gin.Context, which holds the HTTP request data:

• Input Validation 👉 The function first pulls clerk_id from the URL path. If it's missing (results in an empty string "" as shown in the previous section), it immediately stops and returns a 400 Bad Request.
• Context Setup 👉 It retrieves the database connection (db) from the Gin context. A key architectural decision we will revisit.
• User Retrieval 👉 It executes a GORM query (db.Where().Take()) to find the user based on the clerk_id. If an error occurs, it returns a 404 Not Found.
• Data Cleanup (The 'nan' Fix) 👉 It checks the user's Number field. If it's empty, it sets the value to the literal string "nan".
• Company Retrieval 👉 It executes a raw SQL query (db.Raw()) using the user's CompanyID to fetch related company details from the onboarding_table.
• Error Fallback 👉 If the raw SQL query fails (for any reason), it logs the error but sets the company details to default strings like "Unknown", ensuring the function continues.
• Response Assembly 👉 It maps the retrieved fields into two clean response structs: UserDetails and CompanyDetails.
• Final Output 👉 It combines the structs and returns a 200 OK response to the client.

If you are coming from languages like JavaScript or Python, you might expect a missing URL parameter to result in a null or undefined value in the clerkID variable. However, this is simply how Go works with value types like string. When Gin fails to find a path parameter, it returns the zero value for the string type, which is the empty string "".

package main

import "fmt"

func main() {
    // 1. Declare a string variable without initializing it.
    var s string
    
    // 2. Declare an integer variable without initializing it.
    var i int
    
    // 3. Check the values and types.
    fmt.Printf("Default value of uninitialized string s: [%s]\n", s) // prints “”
    fmt.Printf("s is empty string? %t\n", s == "") // prints true
    
    fmt.Println("---------------------------")
    
    fmt.Printf("Default value of uninitialized int i: %d\n", i) // prints 0
    fmt.Printf("i is zero? %t\n", i == 0) // prints true
}

The check clerkID == "" correctly catches the scenario where the path parameter is missing because Gin returns the string's zero value. We are checking for a missing value, not a nil reference, which is standard Go practice. The implementation here is correct, even if the result ("") is surprising to developers from other language backgrounds.

The Roast

Offense 1: The Naked Query (Raw SQL in the Handler)

The biggest architectural sin here is the presence of raw SQL within an HTTP handler. This is the equivalent of wearing a bathrobe to a black-tie event, it's messy, inappropriate, and shows a profound lack of respect for architectural boundaries.

companyQuery := "SELECT onboarding_id, company_name, industry_name, company_size, website_url FROM public.onboarding_table WHERE portal_id = ?"
if err := db.Raw(companyQuery, user.CompanyID).Scan(&company).Error; err != nil {
    // ...
}

Your HTTP handler has become a nosy Database Administrator who thinks they're too good for the ORM. Controllers don't query. Period. This monstrosity violates the Single Responsibility Principle (SRP) so badly, it should lose its job.

By hard-coding the SQL string, you've tattooed your database schema onto the web server layer. If a DB admin renames onboarding_table to company_info, your entire API breaks. You are now redeploying a Gin microservice because of a simple table name change. That's not microservices, that's a distributed monolith built on magic strings and poor life choices.

Stop treating your controller like a scrap paper notepad! The database logic must be encapsulated.

1. Introduce a Data Access Layer (DAL) / Repository Pattern. The handler calls a service method, and the service handles the data. This breaks the toxic coupling.
2. If you must use raw SQL (which you shouldn't), bury the string deep in a repository constant. Ideally, convert this to GORM's type-safe db.Model().Where().Select() methods.

Security Note (For the Smart Alecks): For those thinking "SQL Injection!", calm down. The ? placeholder in db.Raw() saves your bacon by using parameter binding. The issue here isn't a security flaw; it's an Architectural Crime against maintainability and separation of concerns. You're safe from hackers, but you're vulnerable to me.

Offense 2: The Ambiguous 404/500 (Catching All Errors)

This code is pathologically incapable of honesty. It uses a 404 Not Found as a blanket excuse for every infrastructure failure, confusing the client and hiding genuine disasters from monitoring tools.

var user models.User
if err := db.Where("clerk_id = ?", clerkID).Take(&user).Error; err != nil {
    logger.GetLogger().Errorf("Failed to retrieve user profile: %v", err)
    c.JSON(http.StatusNotFound, ErrorResponse{ // Always returns 404
        Error: "User not found",
    })
    return
}

Oh, look! The database crashed, the connection timed out, or maybe a rogue commit just dropped the entire user table. And what does our brave little handler do? It shrugs, lies to the client, and smugly returns 404: "User not found."

This isn't helpful error handling; it's operational sabotage! You're taking a critical 500 Internal Server Error, which should trigger PagerDuty alarms and wake up the SRE team, and disguising it as a benign 404 Client Error. Your monitoring dashboards are pristine, your log file is a graveyard, and the database is actively on fire.

404 is a contract: "The thing you asked for doesn't exist." It is not a blanket code for "I tried, but something broke."

Be honest with your errors! You must explicitly check the underlying cause using errors.Is to distinguish between a semantic failure and an infrastructural one.

if err := db.Where("clerk_id = ?", clerkID).First(&user).Error; err != nil {
	if errors.Is(err, gorm.ErrRecordNotFound) {
		// 🤝 Honesty: The record doesn't exist.
		c.JSON(http.StatusNotFound, ErrorResponse{Error: "User not found"})
	} else {
		// 🚨 Accountability: The database connection failed, or the query syntax is trash.
		logger.GetLogger().Errorf("CRITICAL DB FAILURE: %v", err)
		c.JSON(http.StatusInternalServerError, ErrorResponse{Error: "Internal server error retrieving user data"})
	}
	return
}

If your code can't tell the difference between a missing user and a catastrophic database failure, you're not writing an API; you're writing a highly available lie detector.

Offense 3: The Placeholder Value ("nan")

This little piece of code is a control freak trying to enforce presentation standards on the frontend team with a confusing, technically illiterate placeholder.

number := user.Number
if number == "" {
    number = "nan"
}
// ...
userDetails := UserDetails{
    Number: number, // "nan" is used here

Your API is not a web browser! Why are you injecting the garbage string "nan" (Not a Number, for a field that is supposedly a string!) into the response? You have officially committed the sin of Backend Presentation Dictatorship.

You should send a clean empty string ("") to signify "No Value Here." Why? Because it is the client's job to decide whether they should display "N/A", "Not Provided" or a little dancing GIF.

If tomorrow the frontend team decides they should display "Call us for details!" instead of "nan", are you going to redeploy your entire API server just to change a fallback string? Absolutely not!

Stop polluting your data payload with arbitrary, confusing strings.

1. Use the Zero Value: The correct signal for "no value" in a JSON API is typically the field's zero value. For a string, that is a clean "".
2. Use Explicit Constants (if needed): If you absolutely must use a fallback string for comparisons, use a clean, predictable, all-caps constant like "UNSPECIFIED" that the client can easily === check, rather than a misleading acronym stolen from floating-point arithmetic.

For maximum clarity and minimum client confusion, simply pass the empty string:

// No change needed! Just let the empty string be the empty string.
number := user.Number 
// ...
userDetails := UserDetails{
    Number: number, // If user.Number is "", the output is ""
// ...

Don't be a micromanager; send the data and let the frontend do its job.

Offense 4: The Silent Failure (Ignoring Critical Errors)

This error block ensures that even if the database connection snaps in half, the HTTP request succeeds. It's the equivalent of a submarine crew celebrating success while the hull is actively imploding.

if err := db.Raw(companyQuery, user.CompanyID).Scan(&company).Error; err != nil {
    logger.GetLogger().Errorf("Failed to retrieve company information: %v", err)
    company = CompanyDetails{ // <-- Shh! Pretend everything is fine!
        CompanyName: "Unknown", 
        // ...
    }
}

You have created the "Everything is Fine" Anti-Pattern. The server is on fire, but the dog is sitting calmly, wearing a hat. If the entire onboarding_table vanishes, if the database administrator throws a tantrum and revokes query permissions, or if the server explodes, your API still returns a 200 OK with the company name set to the overly dramatic, juvenile string: "Unknown".

You log the error, sure, but you immediately suppress the failure from the calling client. This is operational negligence. You are masking a critical 500 infrastructure error (which affects every user) as a benign data enrichment failure that only affects one user's profile. Your logs will be screaming in the dark while your service reports 100% uptime.

You must apply the same discipline as Offense 2. Differentiate between:

1. Semantic Failure (gorm.ErrRecordNotFound): The company doesn't exist. This is fine; use graceful, professional fallbacks like "Not Provided".
2. Infrastructure Failure (Any Other Error): The database connection is broken. This is a critical 500 error and should halt execution or at least be logged with extreme prejudice and reported to the client as an internal issue.

Be honest with your errors, or your users will find out the hard way when all company names suddenly turn into "Unknown".

Offense 5: The Sneaky Typo (The Unpaid Overtime Trap)

This isn't a runtime error, it's a social failure. A tiny spelling mistake that creates a lifelong burden of maintenance debt.

	userDetails := UserDetails{
		// ...
		ConnectAccounStatus: user.ConnectAccounStatus, // <-- Typo!
		PayoutsEnabled:      user.PayoutsEnabled,

Look here. I know some of you are thinking,

“It’s just a typo, let it go, man.”

But let me ask you something: Do you want to be popular?

Now, do you want yourself to be known as Jimmy Joke instead of Jimmy Joe? It’s just a typo, so no worries, right?

This misspelled field, ConnectAccounStatus, is a future disaster. A misspelled variable that is bound to get used in a thousand places will show up in the faces of countless developers. And here is the real cost: they won't bother fixing it because it's either above their pay grade or, worse, they don’t want to serve unpaid overtime only because they missed fixing the function name in 1 out of 1000 places.

This small mistake breeds technical fear and signals that your API contract, the very promise you make to client applications, is not dependable.

If fixing a typo causes the world to stop, your architecture has already failed. This is a matter of discipline and quality control.

Offense 6: Context Pollution

This code violates the fundamental rule of application structure:

never marry your business logic to your web framework.

db := GetCompanyDBFromContext(c)

You've just married your entire business logic to the Gin web framework! By pulling the raw database connection *gorm.DB directly out of the c *gin.Context, you've achieved a level of tight coupling that should come with a mandatory prenuptial agreement.

This is why we can't have nice things, like unit tests. You can't test this logic without mocking the entire, bloated gin.Context object. Your code is now an arrogant little island that refuses to talk to anyone unless they've arrived via HTTP.

Want to reuse your profile logic in a simple CLI tool, a queue worker, or a scheduled job? Too bad! Your code is inextricably chained to Gin's request lifecycle. This isn't software development; it's Stockholm Syndrome with your framework.

Break up with the Gin context! We use Dependency Injection (DI) for a reason.

Your business logic (User Service or Repository) should be constructed once with the database connection it needs, and then that object is injected into the handler struct. The handler's only job is to receive HTTP, call the injected service, and send back HTTP.

// In the proper setup:
type Handler struct {
    // The dependency is injected at startup, NOT retrieved during the request.
    UserService *UserService 
}

func (h *Handler) GetUserProfile(c *gin.Context) {
    // Look how clean and testable this is!
    profile, err := h.UserService.GetProfile(clerkID) 
    // ...
}

If you can't test a function without spinning up an entire web server, you're not writing unit tests; you're writing integration test excuses!

Offense 7: The Biggest And Baddest Security Gap

This is the security vulnerability that makes CISOs sweat. It instantly turns any authenticated user into a universal data thief. This isn't a bug; it's a data breach waiting to happen.

clerkID := c.Param("clerk_id")
// ... retrieves and returns user profile based on this ID

This is not just a massive security hole, this is a guaranteed ticket to unemployment! If this app were Netflix, you'd be getting sued into oblivion. You've introduced a classic Broken Function Level Authorization (BFLA) flaw.

You have a perfectly good BearerAuth middleware that validates the user's token, confirming who they are. But instead of using the trusted ID from that token, you blindly trust the malicious ID provided by the client in the URL path!

Any low-level authenticated user can now query the endpoint for /profile/some-other-user's-id and steal data until the database lights up.

If anyone asked me, I’d say, "It wasn’t me." And git blame will finally stand up to its name by blaming the right person.

The fact that you already have a middleware to handle authentication makes this mistake unforgivable. You have a security guard at the front door, and you gave the keys to the thief!

You must REJECT the URL path parameter and only retrieve the authenticated user's ID from the context key populated by your successful middleware.

1. Stop Trusting the Client: Remove c.Param("clerk_id") entirely.
2. Trust the Token: Extract the user ID directly from c.Keys (e.g., c.Get("clerkID")). This ID is cryptographically verified and cannot be faked by the client.

The API endpoint should change from /profile/:clerk_id to simply /profile because the ID is implicit in the request header.

func GetUserProfile(c *gin.Context) {
    // REMOVE: clerkID := c.Param("clerk_id")
    
    // FETCH ID FROM THE TRUSTED TOKEN CONTEXT
    authID, exists := c.Get("clerkID") // ID is guaranteed to be real
    if !exists {
        // ... Panic, because the middleware failed its only job.
    }
    
    // Use the ID that is YOU, not the ID that is provided.
    clerkID := authID.(string) 

    // proceed with DB query using clerkID ... or not. Throw all logic in a service or a repository
}

Good thing we review each other’s code... right?

The Verdict: From Ghost to Gold

We started with a fragile, tightly-coupled function, and by the end, we exposed seven critical flaws. Ranging from career-ending security breaches to tedious maintenance traps.

Here is the final tally of the sins we just roasted:

The root of almost every problem here is a violation of two simple rules: Separation of Concerns (Controllers do HTTP, Repositories do DB) and Never Trust the Client (Identity comes from the token, not the URL).

The Final Takeaway: Stop Writing Excuses

The ultimate goal of this roast isn't to be mean (though that was definitely fun). It's to prove that clean, idiomatic code is the most efficient way to work.

Every flaw we found, from the typo that costs future team members overtime to the architectural sin that made unit testing impossible, was solved by adhering to established design patterns. By correctly implementing Dependency Injection and enforcing strict authorization boundaries, we transform this vulnerable, brittle function into a clean, testable, and secure utility.

Clean code isn't a luxury; it's a liability reduction strategy.

-----

What code is currently hiding a fatal flaw in your codebase? Go check your most complicated endpoint, and join me next time for the next session of Code Roast, where we find the next ghost function and turn it into gold.

Join my discord community and show me the code that you want roasted into oblivion.

If this post clicked with you, you'll love the newsletter. It's where I share raw, no-fluff lessons from real projects. The kind of stuff you don't find in documentation.

If you feel like it, my community is always open for like minded engineers who seek to level up their game. You are more than welcome to hang out with us. We talk code, chaos, and the craft of building things that don't break. Join the Discord channel

Lastly, everything I make is powered by you.

If you'd like to keep this campfire burning, you can drop a tip in the Tip Jar … or simply spread the word. Every bit helps.